Skip to content

LambdaRewriter.SubstituteTypeArguments should make progress#37487

Merged
jcouv merged 4 commits intodotnet:masterfrom
jcouv:fix-hang
Jul 26, 2019
Merged

LambdaRewriter.SubstituteTypeArguments should make progress#37487
jcouv merged 4 commits intodotnet:masterfrom
jcouv:fix-hang

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Jul 25, 2019

Fix #37456

@gafter Looking back at #35284, I still think the use of reference equality is correct, but I want to check with you in case you think we should return to the original .Equals(..., ConsiderEverything) comparison.

@jcouv jcouv added this to the 16.3.P2 milestone Jul 25, 2019
@jcouv jcouv requested a review from a team as a code owner July 25, 2019 21:36
@jcouv jcouv self-assigned this Jul 25, 2019
return CreateCompilation(source, options: WithNonNullTypesTrue(), references: references);
}

[Fact, WorkItem(37456, "https://github.com/dotnet/roslyn/issues/37456")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the right file for this test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok because the regression came from nullability work.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the test ;-)

}

[Fact, WorkItem(37456, "https://github.com/dotnet/roslyn/issues/37456")]
public void verify37456()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v [](start = 20, length = 1)

Uppercase first char.

@cston
Copy link
Copy Markdown
Contributor

cston commented Jul 26, 2019

            while ((object)oldTypeArg.Type != newTypeArg.Type);

Consider changing this back to !TypeSymbol.Equals(oldTypeArg.Type, newTypeArg.Type, TypeCompareKind.ConsiderEverything) (as it was before #35284) in case SubstituteType() does return an identical copy rather the original instance. We could assert the instances are identical rather than relying on that.


Refers to: src/Compilers/CSharp/Portable/Lowering/LambdaRewriter/LambdaRewriter.cs:946 in ded9135. [](commit_id = ded9135, deletion_comment = False)

@jcouv jcouv merged commit a000984 into dotnet:master Jul 26, 2019
@jcouv jcouv deleted the fix-hang branch July 26, 2019 20:11
@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Jul 26, 2019

Fix merged for 16.3p2. We're discussing porting it back to a servicing fix for 16.2.

jcouv added a commit to jcouv/roslyn that referenced this pull request Jul 29, 2019
agocke pushed a commit that referenced this pull request Jul 30, 2019
@gafter
Copy link
Copy Markdown
Member

gafter commented Aug 1, 2019

I believe the reference equality is correct; type substitution should not return a new instance when it makes no substitutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VBCSCompiler.exe hangs when building .Net Core 3 Preview 7 app from Visual Studio 16.3 Preview 1

5 participants