-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Several simplifications to TypeSymbolWithAnnotations #28583
Conversation
@@ -138,21 +138,29 @@ internal TypeSymbolWithAnnotations SubstituteType(TypeSymbolWithAnnotations prev | |||
/// In particular, if substitution makes type tuple compatible, transform it into a tuple type. | |||
/// </summary> | |||
internal TypeSymbolWithAnnotations SubstituteTypeWithTupleUnification(TypeSymbol previous) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More general question: why do we have this API, instead of just using SubstituteType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the compiler does not match the language in the way that tuple types are handled. See #20648 which describes a set of changes that are planned to bring them into alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm afraid I may have answered the question you didn't ask)
In reply to: 202755152 [](ancestors = 202755152)
@@ -304,15 +327,20 @@ public bool IsAtLeastAsVisibleAs(Symbol sym, ref HashSet<DiagnosticInfo> useSite | |||
return NullableUnderlyingTypeOrSelf.IsAtLeastAsVisibleAs(sym, ref useSiteDiagnostics); | |||
} | |||
|
|||
public virtual TypeSymbolWithAnnotations SubstituteType(AbstractTypeMap typeMap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual [](start = 15, length = 7)
What's the virtual removal change related to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new protected
overload is now virtual so derived classes have a single method to override rather than two.
Done review pass (commit 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes to
TypeSymbolWithAnnotations
:ToDisplayString
to base classwithTupleUnification
parameter toSubstituteType
LazyNullableTypeParameter.SpecialType
andIsRestrictedType
Code changes separated out from #28453.