Ref returns/locals: Crash (& Debug.Asserts) in Inline Temp

[dpoeschl]

1. Paste C#:

   class C
   {
       ref int M()
       {
           int[] arr = new[] { 1, 2, 3 };
           ref int x = ref arr[2];
           return ref x;
       }
   }

2. Try to inline temp on x

Crash:

    Microsoft.CodeAnalysis.CSharp.Features.dll!Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary.InlineTemporaryCodeRefactoringProvider.DetectSemanticConflicts(Microsoft.CodeAnalysis.Document inlinedDocument, Microsoft.CodeAnalysis.SemanticModel newSemanticModelForInlinedDocument, Microsoft.CodeAnalysis.SemanticModel semanticModelBeforeInline, Microsoft.CodeAnalysis.SymbolInfo originalInitializerSymbolInfo, System.Threading.CancellationToken cancellationToken) Line 519  C#
    Microsoft.CodeAnalysis.CSharp.Features.dll!Microsoft.CodeAnalysis.CSharp.CodeRefactorings.InlineTemporary.InlineTemporaryCodeRefactoringProvider.InlineTemporaryAsync(Microsoft.CodeAnalysis.Document document, Microsoft.CodeAnalysis.CSharp.Syntax.VariableDeclaratorSyntax declarator, System.Threading.CancellationToken cancellationToken) Line 227    C#

[dpoeschl]

@jaredpar @Pilchie Labeling & assigning as requested, but do you want different labeling for IDE failures?

[jaredpar]

I'd label compiler / IDE as if this were a bug we discovered in the working product. Should keep that consistent. Can make a separate decision later on which bugs are fixed by who.

My 2 cents.

[dpoeschl]

@jaredpar @DustinCampbell @Pilchie What's the plan for this? Should this block merging (both in theory and in practice -- I'm okay with different answers for those since this is our first rodeo)?

[Pilchie]

Our stated principle is that it's okay if refactorings around new language features don't work, but they shouldn't crash.

[dpoeschl]

Is that also our "in practice" answer for this case?

Also, I think we need a way to mark these New Language Feature issues as Blocking or not, so when you look at the feature's issue query it's clear what's left to be done before merge. Maybe "Blocking Merge" or something?

[jaredpar]

How about we create a milestone for this: Feature RI. Move blocking bugs into that milestone.

[Pilchie]

If this is the only crash, I'd probably be willing to bend the rules in this case. If there are a bunch of similar cases, I would worry about the experience we're shipping.

If only we had a label to know which issues would block the merge...

[dpoeschl]

@jaredpar That works for me. Optimistically added a "Language Feature Merging" milestone and added this to it. https://github.com/dotnet/roslyn/milestones/Language%20Feature%20Merging

Didn't want to have to explain what "RI" meant, and wanted to be clear that it was for "Language Features", not just general features... but maybe it should be for general features? Luckily Milestones are easily renamed :)

[VSadov]

I have added a workaround for the crash. For now the "inline temp" will not be permitted if a local happens to be byref, so there is no crash.

It seems that even in the absence of the crash the "inline temp" might not always do the right thing since it tries to add casts at use sites (expecting that most will be reduced) and that could cause compile errors in transformed code.

[jaredpar]

Nice! Given that it's not crashing anymore I'll move this out of the blocking bucket.

[dpoeschl]

Expressions in ref contexts can't be expanded in certain ways (casting the outermost expression is illegal, but fully qualifying names is okay), so we'll need to update the simplification engine to take the target context into account. /cc: @DustinCampbell

[DustinCampbell]

This shouldn't be a big deal. We should just update the expander to not add illegal expressions in those contexts. That's what we've done in the past.

[CyrusNajmabadi]

This does not repro for me.