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

Allow fully oblivious types to coexist with nullable-aware base types in partial classes #55861

Merged
merged 7 commits into from
Aug 31, 2021
Merged

Allow fully oblivious types to coexist with nullable-aware base types in partial classes #55861

merged 7 commits into from
Aug 31, 2021

Conversation

RikkiGibson
Copy link
Contributor

Fixes #45960

Some of the changes in the implementation are a bit weird here because we're adding some awareness of the difference between a top-level "non-annotated" base type and a top-level "oblivious" base type. This affects the test PartialBaseTypeDifference_02. If it is found that this doesn't make sense, we could simplify the change.

@RikkiGibson RikkiGibson requested a review from a team as a code owner August 24, 2021 19:35
@AlekseyTs

This comment has been minimized.

Sign in to view
@AlekseyTs

This comment has been minimized.

Sign in to view
@jcouv jcouv self-assigned this Aug 27, 2021
@AlekseyTs

This comment has been minimized.

Sign in to view
AlekseyTs
AlekseyTs approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 7)

jcouv
jcouv reviewed Aug 30, 2021
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Bases.cs
var info = diagnostics.Add(ErrorCode.ERR_PartialMultipleBases, Locations[0], this);
baseType = new ExtendedErrorTypeSymbol(baseType, LookupResultKind.Ambiguous, info);
baseTypeLocation = decl.NameLocation;
reportedPartialConflict = true;

static bool containsOnlyOblivious(TypeSymbol type)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we tend to put local functions at the end of methods. Consider moving to keep main logic flow

jcouv
jcouv approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 7) with a nit/suggestion

@RikkiGibson RikkiGibson merged commit ddd7482 into dotnet:main Aug 31, 2021
@ghost ghost added this to the Next milestone Aug 31, 2021
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
@RikkiGibson RikkiGibson deleted the partial-base-nullability branch September 8, 2021 00:16
@RikkiGibson RikkiGibson mentioned this pull request Sep 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types
Projects
None yet
Milestone
17.0.P4
4 participants
@RikkiGibson @AlekseyTs @jcouv @dibarbet