-
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
Partial properties: diagnostics for mismatch between parts #73250
Partial properties: diagnostics for mismatch between parts #73250
Conversation
b72eec9
to
e2862e2
Compare
4e01d6b
to
6718f15
Compare
@@ -145,7 +145,18 @@ internal sealed override ImmutableArray<string> NotNullWhenFalseMembers | |||
bool isNullableAnalysisEnabled, | |||
BindingDiagnosticBag diagnostics) : | |||
base(containingType, syntax.GetReference(), location, isIterator: false, | |||
MakeModifiersAndFlags(property, propertyModifiers, isNullableAnalysisEnabled)) | |||
MakeModifiersAndFlags( |
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 goal of this change is to make it so the getter symbol in public virtual T Prop => ...
is not treated as introducing any of its own modifiers.
This makes the behavior of APIs like LocalAccessibility
more consistent with properties declared using the accessor list syntax public virtual T Prop { get => ... }
.
itemInspector = static obj => obj switch | ||
{ | ||
null => "<null>", | ||
string s => $@"""{s}""", |
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.
This makes it easier to paste baselines in from an AssertEx.Equal failure, since the string values will already be quoted.
I didn't bother to do escaping here, if anything I would like to output a """
-string only if the string is detected to contain any characters that need escaping. It's something which can be addressed when we hit it.
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.
This introduces a problem where every line of a VerifyIL baseline is quoted. I'll have to look at either adjusting that or reverting this change.
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/PartialPropertiesTests.cs
Outdated
Show resolved
Hide 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.
Done with review pass (iteration 9)
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 review pass
|
||
[Fact] | ||
public void TypeDifference_02() | ||
{ |
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 a test like this:
private partial int P1 { get; set; }
partial int P1 { private get => 1; private set {} }
public partial string? M2() => ""; | ||
} | ||
"""; | ||
|
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.
Please test in
vs ref readonly
with indexer parameters.
src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
Show resolved
Hide 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.
Done with review pass (iteration 16)
…erty explicit implementation
[Fact] | ||
public void ModifierDifference_Accessibility_ExplicitDefault_01() | ||
{ | ||
// only one part explicitly specifies the default accessibility |
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.
Do we have a similar test (default accessibility vs. explicit accessibility) for accessors?
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 think test ModifierDifference_Accessibility_ExplicitDefault_02
is exercising the scenario adequately because it shows that:
- explicitly specifying the default accessibility on an accessor is an error
- a difference in accessor accessibility modifiers between parts is an error
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 19). Only one test suggestion left
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 19)
176000a
into
dotnet:features/partial-properties
Test plan: #73090