-
Notifications
You must be signed in to change notification settings - Fork 4.2k
More extension operators declaration work #78457
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
Conversation
@jcouv, @dotnet/roslyn-compiler For the second review |
|
||
this.CheckUnsafeModifier(declarationModifiers, diagnostics); | ||
|
||
if (isCompoundAssignmentOrIncrementAssignment) |
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.
Is this covered by test? #Closed
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.
Is this covered by test?
I am not sure what this question is about. There is no change at this position in the latest revision. Consider reviewing the latest revision. I had to do a merge to resolve a conflict. Apparently CodeFlow is now going crazy on merge commits and showing changes from them as undone in previous commits.
case RefKind.In: | ||
case RefKind.RefReadOnlyParameter: | ||
// 'in' and 'ref readonly' receivers are disallowed for anything that is not a concrete struct (class or a type parameter) | ||
if (extensionParameter.Type is { TypeKind: not TypeKind.TypeParameter, IsValueType: true }) |
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.
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.
Perhaps
{ | ||
if (extensionParameter.Name == "") | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_InstanceMemberWithUnnamedExtensionsParameter, _location, Name); |
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.
Is this covered by test?
This is the line I was trying to point to (producing ERR_InstanceMemberWithUnnamedExtensionsParameter
diagnostic) #Closed
{ | ||
extension(ref S1? s1) | ||
{ | ||
public void operator {{{op}}}() {} |
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.
nit: consider adding number comments (// N
) to make it easier to review this long test. Same below in CompoundAssignment_001_Declaration
#Closed
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.
consider adding number comments
I am using #line
directives, not planning to use comments because they are creating a maintenance issues since some devs confuse them with a form of a base-line
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.
Done with review pass (iteration 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.
Done with review pass (iteration 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 Thanks (iteration 3)
return true; // An error scenario | ||
} | ||
|
||
if (checkStrippedType && type.IsNullableType()) |
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.
We should keep track of this and include in a spec update. Also applies to the proposed validation rules (marked PROTOTYPE in other file) #Resolved
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.
We should keep track of this and include in a spec update
We certainly can re-confirm the behavior, but it follows the current speclet, which doesn't prevent declaration of operators for Nullable<T>
. The other place (marked PROTOTYPE in other file) is adding restrictions that are not mentioned in the speclet
Relates to test plan #76130