Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust type substitution for oblivious type argument in not-annotated type parameter #48694

Closed
wants to merge 7 commits into from
Closed

Adjust type substitution for oblivious type argument in not-annotated type parameter #48694

wants to merge 7 commits into from

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Oct 16, 2020 •
edited
Loading

This PR changes type substitution from:
image

to:
image

Benefits:

  1. we don't need to query for IsTypeParameterDisallowingAnnotationInCSharp8 or IsValueType for type substitution anymore
  2. makes the logic much easier to follow and reason about, and symmetrical
  3. incidentally fixes a couple of issues (fixes Missing warnings on overriding a method with different nullability attributes #41368, fixes Follow up on comments in ImplicitConversions_07 unit-test #29898, fixes No warning when overriding method with out T? rather than out T #49131)

Drawbacks:

  1. it's a change
  2. it reveals a problem with analysis of using foreach which needs further investigation (fixed in this PR, commit 90d4209)
  3. it reveals a problem with analysis of generic user-defined conversions in invocations (fixed in this PR)
  4. it reveals a problem with analysis of generic user-defined conversions in binary operations

Note that iteration 1 just adds a test baseline (TypeSubstitution) to show the change.

@jcouv jcouv self-assigned this Oct 16, 2020
AlekseyTs
AlekseyTs reviewed Oct 17, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/TypeWithAnnotations.cs
{
if (!typeSymbol.IsTypeParameterDisallowingAnnotationInCSharp8())
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 17, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

IsTypeParameterDisallowingAnnotationInCSharp8 [](start = 32, length = 45)

Where else do we use this helper and whether we still need to use it, given that we now allow annotations on type parameters like that? #Resolved

Copy link
Member Author

@jcouv jcouv Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Unless it's crucial for this discussion or PR, I'd rather not branch out to that question. There are many other places that use this method at the moment.

In reply to: 506777623 [](ancestors = 506777623)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Oct 17, 2020 •
edited by jcouv
Loading 

            //         Oblivious2.s.Item /*T:string!*/ = null; // 6

Is this a breaking change for C# 8? #ByDesign

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:9756 in f8e19e3. [](commit_id = f8e19e3f042f5005efaa0d125dfac22e79d704ad, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Oct 17, 2020 

            //         Oblivious2.s.Item /*T:string!*/ = null; // 6

Yes. It is a new nullability warning.

In reply to: 710720527 [](ancestors = 710720527)

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:9756 in f8e19e3. [](commit_id = f8e19e3f042f5005efaa0d125dfac22e79d704ad, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Oct 19, 2020 •
edited
Loading

This draft PR for proposal 2 reveals some annoying additional warnings. We're now leaning towards proposal 0 or proposal 1 (doc). Will open a new PR for one of these

@jcouv jcouv closed this Oct 19, 2020
@jcouv jcouv reopened this Nov 3, 2020
@jcouv jcouv marked this pull request as ready for review November 10, 2020 18:20
@jcouv jcouv requested a review from a team as a code owner November 10, 2020 18:20
@jcouv
Copy link
Member Author

jcouv commented Nov 10, 2020

@cston @AlekseyTs @dotnet/roslyn-compiler This PR can be looked at. I've walked Chuck and Aleksey through the design change, but am happy to do the same for anyone interested.

I've fixed two of the orthogonal issues (re-inferring user-defined conversions, analysis of await foreach). There's one issue remaining with binary operators. I'll see if I can fix it as well, or separate it out into a follow-up issue.

@jcouv jcouv requested a review from AlekseyTs November 10, 2020 21:28
@jcouv jcouv marked this pull request as draft November 11, 2020 00:27
This was referenced Nov 11, 2020
Merged
Merged
@jcouv jcouv force-pushed the type-substitution branch 2 times, most recently from cb1daa8 to f941ea6 Compare November 13, 2020 21:47
@runfoapp runfoapp bot mentioned this pull request Nov 14, 2020
Closed
@jcouv jcouv force-pushed the type-substitution branch from 5ea623b to b1b73ee Compare November 16, 2020 19:30
@runfoapp runfoapp bot mentioned this pull request Nov 17, 2020
Closed
@runfoapp runfoapp bot mentioned this pull request Nov 20, 2020
Open
@jcouv
Copy link
Member Author

jcouv commented Dec 2, 2020

Closing in favor of #49723

@jcouv jcouv closed this Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs Awaiting requested review from AlekseyTs

Assignees

@jcouv jcouv

Labels
Area-Compilers Feature - Nullable Reference Types Nullable Reference Types
Projects
None yet
Milestone
No milestone
2 participants
@jcouv @AlekseyTs