-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Allow record's ToString method to be sealed #52029
Conversation
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
Show resolved
Hide resolved
Please add an entry to Thanks #Closed |
Thanks for taking this feature. Ping when ready for another look or if you have any questions. #Resolved |
Some tests are failing, but they don't seem related to my changes... flaky tests? Or did I break something? |
Failures look flaky. Let's rerun the failing test jobs. |
); | ||
} | ||
|
||
[Fact] | ||
public void ToString_DerivedRecord_BaseHasSealedToString_PreviewSealedRecordToString() |
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: could be merged into preview test, since same source string (create a compilation and check diagnostics, then repeat with different options). Same applies to tests below.
Please verify execution (using CompileAndVerify(comp, expectedOutput: "...")
) as well to show that we didn't generate a new ToString.
Please also add a test where the sealed ToString comes from the base's base (to cover the while
loop in getBaseToStringMethod
).
Also with some other ToString
methods in base type (parameterlist not empty, return type not string) to cover the matching logic in getBaseToStringMethod
. #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.
could be merged into preview test
Do you mean changing the test to a Theory
instead of Fact
? #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.
Also with some other ToString methods in base type (parameterlist not empty, return type not string)
I'm unable to introduce a ToString()
method with a different return type. I get this error:
error CS8869: 'C2.ToString()' does not override expected method from 'object'.
So I'll just add a method with non-empty parameter list.
(Not sure what this CS8869 error is, btw; I don't get this error if I do the same thing in a class instead of a record. What's the rationale behind this?) #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.
Do you mean changing the test to a Theory instead of Fact?
I had a typo. Meant into "previous" test:
string src = "...";
var comp = CreateCompilation(..., Regular9);
comp.VerifyEmitDiagnostics(...);
comp = CreateCompilation(..., RegularPreview);
comp.VerifyEmitDiagnostics();
CompileAndVerify(comp, expectedOutput: "..."); // can verify execution when no error in LangVer=preview
``` #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.
I'm unable to introduce a ToString() method with a different return type.
It's possible, but requires jumping through some hoops by defining your own System.Object
type. See RecordTests.ObjectEquals_05
for help doing that. The key is to use CreateEmptyCompilation
to avoid pulling libraries that already have this type. #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.
I had a typo. Meant into "previous" test
Yes, I gathered as much.
Is it OK that I used a theory instead of creating compilations with different options in the same 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.
It's possible, but requires jumping through some hoops by defining your own System.Object type. See RecordTests.ObjectEquals_05 for help doing that. The key is to use CreateEmptyCompilation to avoid pulling libraries that already have this type.
Thanks, I'll look into it. Not today, though...
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.
Added a test for this scenario (ToString_DerivedRecord_BaseBaseHasSealedToString_And_BaseHasToStringWithDifferentReturnType). I no longer needed to jump through hoops, thanks to the changes related to CS8869
Diagnostic(ErrorCode.ERR_CantOverrideSealed, "B").WithArguments("B.GetHashCode()", "A.GetHashCode()").WithLocation(7, 8) | ||
); | ||
|
||
var actualMembers = comp.GetMember<NamedTypeSymbol>("B").GetMembers().ToTestDisplayStrings(); |
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.
It'll be good to factor the C# 9 and preview tests into one so that we can share the validation logic (showing that the expectations are the same). #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.
Done with review pass (iteration 4). It's looking good modulo some test compaction and testing a couple more scenarios.
Just a heads up on process for compiler features:
|
@jcouv I think I addressed all comments from your last review
Ah, yes, that could be a problem... but if you try to do the same with records instead of classes, you get this:
So I don't think this situation can actually occur |
|
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 5)
@@ -6600,4 +6600,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ | |||
<data name="ERR_FunctionPointerTypesInAttributeNotSupported" xml:space="preserve"> | |||
<value>Using a function pointer type in a 'typeof' in an attribute is not supported.</value> | |||
</data> | |||
<data name="IDS_FeatureSealedRecordToString" xml:space="preserve"> | |||
<value>sealed record ToString</value> |
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.
sealed record ToString [](start = 11, length = 22)
sealed record ToString [](start = 11, length = 22)
"sealed ToString in record"? #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.
Done
@@ -324,6 +325,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature) | |||
{ | |||
// C# preview features. | |||
case MessageID.IDS_FeatureMixedDeclarationsAndExpressionsInDeconstruction: | |||
case MessageID.IDS_FeatureSealedRecordToString: |
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.
case MessageID.IDS_FeatureSealedRecordToString: [](start = 16, length = 47)
case MessageID.IDS_FeatureSealedRecordToString: [](start = 16, length = 47)
I am assuming this is a semantic check. Consider adding a comment. #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.
Done
.SingleOrDefault(m => m.ParameterCount == 0 && m.ReturnType.SpecialType == SpecialType.System_String); | ||
|
||
if (method is not null) | ||
return method; |
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.
return method; [](start = 28, length = 14)
return method; [](start = 28, length = 14)
The fact that the name and the signature match doesn't mean this is an override of Object.ToString. We should be able to come up with a test scenario with base like that. If not defined in source, then in IL. #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.
I think we should mimic the check that is performed by SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject
, but I would also simplify its implementation by switching to GetSpecialTypeMember helper to locate the Object member and comparing it with leastOverridden
instead of performing manual signature comparison.
In reply to: 601500624 [](ancestors = 601500624)
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.
FYI, I just merged a refactoring for SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject
to take advantage of SpecialMemeber
.
In reply to: 601544539 [](ancestors = 601544539,601500624)
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.
@AlekseyTs I made some changes to getBaseToStringMethod
(compared least overriden to object.ToString(), replaced Linq usage with a loop). Is this what you had in mind?
{ | ||
var toStringMethod = new SynthesizedRecordToString(this, printMethod, memberOffset: members.Count, diagnostics); | ||
members.Add(toStringMethod); | ||
} | ||
} | ||
else | ||
{ | ||
var toStringMethod = (MethodSymbol)existingToStringMethod; | ||
if (!SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject(toStringMethod, SpecialType.System_String, diagnostics) && toStringMethod.IsSealed && !IsSealed) |
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.
!SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject(toStringMethod, SpecialType.System_String, diagnostics) [](start = 24, length = 118)
!SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject(toStringMethod, SpecialType.System_String, diagnostics) [](start = 24, length = 118)
If we allow sealing Object.ToString up the hierarchy, it feels like we should allow to declare new
ToString method with the same parameter types that doesn't override Object.ToString, but shadows it instead. #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.
I mean in cases when the override of Object.ToString is sealed up the hierarchy.
In reply to: 601507591 [](ancestors = 601507591)
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.
If we allow sealing Object.ToString up the hierarchy, it feels like we should allow to declare new ToString method with the same parameter types that doesn't override Object.ToString, but shadows it instead.
Why is it currently forbidden in records anyway?
Anyway, wouldn't it be out of scope for this PR? I set out to allow sealing ToString()
, not remove a restriction I didn't even know about...
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.
If we allow sealing Object.ToString up the hierarchy, it feels like we should allow to declare new ToString method with the same parameter types that doesn't override Object.ToString, but shadows it instead.
I mean in cases when the override of Object.ToString is sealed up the hierarchy.
Why is it currently forbidden in records anyway?
Anyway, wouldn't it be out of scope for this PR? I set out to allow sealing ToString(), not remove a restriction I didn't even know about...
This wasn't specifically forbidden. However, there was a requrement that a string ToString()
method added explicitly overrides Object.ToString
. We are lifting this requirement for cases when Object.ToString
is sealed in a base class. Therefore, when one adds string ToString()
method in a situation like that, compiler shouldn't require that that method overrides Object.ToString
, because this simply cannot be done. It is not posible to override a sealed method. Please create a unit-test for this scenaro, I think that should give you the idea what I am talking about.
In reply to: 602653210 [](ancestors = 602653210)
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 create a unit-test for this scenaro, I think that should give you the idea what I am talking about.
@AlekseyTs do you mean something like this?
[Fact]
public void ToString_NewToString_SealedBaseToString()
{
var source = @"
public record A
{
public sealed override string ToString() => throw null;
}
public record B : A
{
public new string ToString() => throw null;
}";
var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview);
comp.VerifyEmitDiagnostics();
}
This currently fails with this error:
// (9,23): error CS8869: 'B.ToString()' does not override expected method from 'object'.
// public new string ToString() => throw null;
Should I make this test pass, then?
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 you mean something like this?
Yes.
Should I make this test pass, then?
I think yes.
In reply to: 602934953 [](ancestors = 602934953)
{ | ||
var method = tmp.GetSimpleNonTypeMembers(WellKnownMemberNames.ObjectToString) | ||
.OfType<MethodSymbol>() | ||
.SingleOrDefault(m => m.ParameterCount == 0 && m.ReturnType.SpecialType == SpecialType.System_String); |
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.
SingleOrDefault [](start = 29, length = 15)
SingleOrDefault [](start = 29, length = 15)
In metadata we could have more than one method with the same return type, but with different custom modifiers. Probably we should not assume there could be only one candidate, but instead check all of them. #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.
@AlekseyTs the new matching logic (using least overridden) accounts for that, right?
while (tmp is not null) | ||
{ | ||
var method = tmp.GetSimpleNonTypeMembers(WellKnownMemberNames.ObjectToString) | ||
.OfType<MethodSymbol>() |
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.
OfType [](start = 29, length = 6)
OfType [](start = 29, length = 6)
I think a regular loop through members would be preferred other the Linq query in this case. #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.
Done
var comp = CreateCompilationWithIL( | ||
new[] { source, IsExternalInitTypeDefinition }, | ||
ilSource: ilSource, | ||
parseOptions: usePreview ? TestOptions.RegularPreview : TestOptions.Regular9); |
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.
TestOptions.Regular9 [](start = 72, length = 20)
I think we should report a language version error for Regular9
because previous compiler wouldn't be able to compile this code and targeting that version should prevent a build break like that when compiler is switched back. #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.
Sorry, maybe I'm a bit slow (it's 3 a.m. here), but I don't get it... Could you elaborate?
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 we should report a language version error for Regular9 because previous compiler wouldn't be able to compile this code and targeting that version should prevent a build break like that when compiler is switched back.
Sorry, maybe I'm a bit slow (it's 3 a.m. here), but I don't get it... Could you elaborate?
There is a requirement - If compiling some code with a particular language version succeeds, then compiling the same code by using an old version of the compiler that added support for that language version should also succeed and result should be equivalent. This is an error scenario in the previous version of the compiler. Therefore, when we compile this code with language version C# 9, we should report an error, saying that the language version should be bumped up.
In reply to: 602653525 [](ancestors = 602653525)
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.
So, just to make sure I understand it right... If language version is 9, it should:
- not emit a
ToString()
override (since the method is sealed in the base type) - emit a
ERR_FeatureInPreview
diagnostic
Is that correct?
I just worry that the error will be a bit confusing. The user would just be trying to inherit a record with a sealed ToString (e.g. from another assembly compiled with C# 10), but the error will say that "sealed ToString is records" is a preview feature.
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.
So, just to make sure I understand it right... If language version is 9, it should:
- not emit a ToString() override (since the method is sealed in the base type)
- emit a ERR_FeatureInPreview diagnostic
Is that correct?
I just worry that the error will be a bit confusing. The user would just be trying to inherit a record with a sealed ToString (e.g. from another assembly compiled with C# 10), but the error will say that "sealed ToString is records" is a preview feature.
If the simple error will be confusing, we should consider adding a dedicated more descriptive error, which will request to bump the language version. For example: "Inheriting from a class with a sealed 'Object.ToString' is not supported in C# 9. Please use language version 'preview' or greater."
In reply to: 602872811 [](ancestors = 602872811)
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.
OK, I addressed that in commit 7dcf815. (also added the new error message)
[Fact] | ||
public void ToString_DerivedRecord_BaseHasSealedToString() | ||
public void ToString_DerivedRecord_BaseBaseHasSealedToString() |
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.
ToString_DerivedRecord_BaseBaseHasSealedToString [](start = 20, length = 48)
I think we should add more tests for:
- trying to override the sealed method explicitly and how next level derived records are going to behave.
- trying to shadow method, including in a way when it will not be accessible in derived types, including when shadowed method is not sealed.
- inheritance behvior in presence of shadowing up the hierarhy #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.
- trying to override the sealed method explicitly and how next level derived records are going to behave.
Done in ToString_DerivedRecord_BaseBaseHasSealedToString_And_BaseTriesToOverride
- trying to shadow method, including in a way when it will not be accessible in derived types, including when shadowed method is not sealed.
Done in ToString_DerivedRecord_BaseBaseHasSealedToString_And_BaseShadowsToStringPrivate and ToString_DerivedRecord_BaseBaseHasSealedToString_And_BaseShadowsToStringNonSealed
- inheritance behvior in presence of shadowing up the hierarhy
I think this is already covered in ToString_NewToString_SealedBaseToString
Done with review pass (commit 5) #Closed |
@jcouv sorry, I don't get it. What test should I add? |
And don't generate ToString if base implementation is sealed
var baseToStringMethod = getBaseToStringMethod(); | ||
bool isBaseToStringSealed = baseToStringMethod is { IsSealed: true }; | ||
|
||
if (isBaseToStringSealed) |
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.
isBaseToStringSealed [](start = 20, length = 20)
Since this local is used only in one place, consider inlining baseToStringMethod is { IsSealed: true }
condition here, then we wouldn't need to supress nullable warnings when baseToStringMethod
is dereferenced. #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.
Done #Closed
} | ||
} | ||
"; | ||
var source = @" | ||
var b = new B(); | ||
System.Console.Write(b.ToString()); |
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.
Did you mean to execute code in success branch below? #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.
Oops, yes I did.
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
"; | ||
|
||
var comp = CreateCompilation(src, parseOptions: TestOptions.RegularPreview, options: TestOptions.DebugExe); | ||
comp.VerifyEmitDiagnostics(); |
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.
comp.VerifyEmitDiagnostics(); [](start = 12, length = 29)
I think it will be good to assert that there is no ToString method in C3. #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.
Will do
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
"; | ||
|
||
var comp = CreateCompilation(src, parseOptions: TestOptions.RegularPreview, options: TestOptions.DebugExe); | ||
comp.VerifyEmitDiagnostics(); |
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.
comp.VerifyEmitDiagnostics(); [](start = 12, length = 29)
I think it will be good to assert that there is no ToString method in C3. #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.
Done
Done with review pass (commit 19) #Closed |
{ | ||
var toStringMethod = new SynthesizedRecordToString(this, printMethod, memberOffset: members.Count, diagnostics); | ||
members.Add(toStringMethod); | ||
if (baseToStringMethod!.ContainingModule != this.ContainingModule && !this.DeclaringCompilation.IsFeatureEnabled(MessageID.IDS_FeatureSealedToStringInRecord)) |
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.
! [](start = 42, length = 1)
Is the suppression still needed? #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.
No, it's not. I removed it. #Closed
[Fact] | ||
public async Task UpgradeProjectForSealedToStringInRecords_CS8912() | ||
{ | ||
await TestLanguageVersionUpgradedAsync( |
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.
TestLanguageVersionUpgradedAsync [](start = 18, length = 32)
Did you confirm that the test fails without changes in CSharpUpgradeProjectCodeFixProvider? #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.
Yes. It failed with "No action was offered when one was expected" #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.
LGTM (commit 23) with a couple of questions.
@jcouv Please review the latest revision. |
@dotnet/roslyn-ide Please review IDE side of changes |
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 24)
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 24)
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Merged/squashed. Thanks @thomaslevesque for the contribution! |
Thanks for the review @jcouv and @AlekseyTs! |
And don't generate ToString if base implementation is sealed
Fixes dotnet/csharplang#4174