Change name of record Clone method to unspeakable#44852
Change name of record Clone method to unspeakable#44852agocke merged 1 commit intodotnet:features/recordsfrom
Conversation
There was a problem hiding this comment.
It looks like there is also a PROTOTYPE comment on line 53, do we still consider it to be relevant or is overriding/hiding Clone an unsupported scenario?
There was a problem hiding this comment.
I think it's half-relevant in that I haven't thought about hiding. But it should handle overriding -- at least, my understanding of LookupMembersInType is that it will stop looking through base types when it finds a candidate in a derived type.
f656c76 to
26adbca
Compare
There was a problem hiding this comment.
For other unspeakable names, don't we use a key letter (such as "k" for backing fields)?
internal static string MakeBackingFieldName(string propertyName)
{
Debug.Assert((char)GeneratedNameKind.AutoPropertyBackingField == 'k');
return "<" + propertyName + ">k__BackingField";
}
There was a problem hiding this comment.
Yeah, but those examples are generated names where there's a user-dependent component. This is always the same name, so I didn't think it was particularly hard to reverse engineer if we needed to.
I couldn't find a test like this (attempting Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:1000 in 26adbca. [](commit_id = 26adbca7ce47f559e611b2e30f1dc93dabee7dec, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 1)
There was a problem hiding this comment.
diagnostics.Add(ErrorCode.ERR_BadRecordBase, baseLocation); [](start = 16, length = 59)
I think this is going to break some tests added in #44876 #Closed
There was a problem hiding this comment.
if (!field.IsStatic) [](start = 16, length = 20)
I opened a #44879 for this bug. #Closed
|
@jcouv I didn't understand your comment about WithExpr19 |
|
@RikkiGibson Do you have a chance to take a look at this? |
|
Taking a look. |
src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
nit: these diagnostic comments seem out of date, which makes interpreting the test a bit confusing.
There was a problem hiding this comment.
Which one are you thinking of?
// (5,12): error CS0527: Type 'C' in interface list is not an interface
// struct F : C { }
this looks right, structs can only have interfaces in the base list.
There was a problem hiding this comment.
for example, E is a record type in the source, but the diagnostic comments make mention of an interface E. Just deleting the expected diagnostics and pasting the baselines back in would fix it.
There was a problem hiding this comment.
useSiteDiagnostics [](start = 20, length = 18)
Could use-site diagnostics be the source of the error here?
There was a problem hiding this comment.
Convert [](start = 37, length = 7)
Did we check that the conversion exist? Are user-defined conversions going to be handled here? I think we should probably include conversion information into IOperation node and apply conversion in CFG rewrite when necessary. We don't have to do all this for the preview, but should capture things to follow up on.
There was a problem hiding this comment.
IsVirtual => true; [](start = 29, length = 18)
IsOverride and IsVirtual cannot be both true. #Closed
There was a problem hiding this comment.
return method; [](start = 24, length = 14)
Post preview we should probably deal with ambiguity cases. Two or more methods match
There was a problem hiding this comment.
IsVirtual: true, [](start = 24, length = 16)
This is not going to be true for an override. #Closed
There was a problem hiding this comment.
IsOverride [](start = 29, length = 10)
It looks like the Clone method is now always called virtually, we should reflect that in CFG. If any additional fix-ups will be necessary for this PR, consider adjusting this #44712 (review) #Closed
|
Done with review pass (iteration 1). I think there is a feedback actionable for the preview. Doesn't have to be addressed in this PR though, but shouldn't be lost. |
|
These seemed important enough to address now and there are merge conflicts to resolve anyway. |
The compiler now uses a reserved name for the clone method which only records will generate. Some existing tests are interesting if we ever allow user-generation of the clone method but cannot be run in the current state. I've left them in, but marked them skipped.
The compiler now uses a reserved name for the clone method which only
records will generate. Some existing tests are interesting if we ever
allow user-generation of the clone method but cannot be run in the
current state. I've left them in, but marked them skipped with the
text "unspeakable clone."