Why did you choose this over CLRSignatureCompareOptions?

Hmm the TypeCompareKind here appears to be different than how we verify parameters. For example tuple element names matter in parameter names. Unless there is a good reason otherwise think we should use the same comparisons for return type and parameters.

I used SourceMemberContainerSymbol.CheckOverrideMember as a basis for this code:

Think that makes sense in overrides because:

1. It's historically what we did so pretty much had to maintain it for consistency when adding new features. Basically it would be weird if we didn't error for dynamic vs. object but did error for IntPtr vs. nint.
2. It's expected that the developer doesn't control the signature of the base member. Hence we should give them the flexbility to use dynamic in their code base because it makes sense while in the base case object was the more natural choice. Forcing them to use object in the override would just result in them adding a bunch of unnecessary casts.

For this feature neither is a concern. This is a new feature so compat doesn't come into play and the developer controls both definitions.

Happy to hear if others disagree but my instinct is to say that parameters and return types should have the same comparisons done here.

I may have misunderstood your comment, but it looks like CLRSignatureCompareOptions does not detect a difference between object/dynamic or IntPtr/nint. It looks like constructing a scenario which requires CLRSignatureCompareOptions involves custom modifiers, array sizes or lower bounds, which I am not sure how to do in source. Do you have any ideas for scenarios which illustrate the need for CLRSignatureCompareOptions?

It's expected that the developer doesn't control the signature of the base member.

It feels like this is also the case to a significant degree when one of the 'partial' declarations comes from a code generator.

I may have misunderstood your comment, but it looks like CLRSignatureCompareOptions does not detect a difference between object/dynamic or IntPtr/nint.

The suggestion of CLRSignatureCmopareOptions was just an example, didn't mean to suggest it should be used.

It feels like this is also the case to a significant degree when one of the 'partial' declarations comes from a code generator.

True but it is also something that it's reasonable for the source generator to replicate. Consider that if they don't replicate what the user wrote, particularly the tuple element names, then they're subverting the will of the developer and making their generated code much harder to consume.

Consider that if they don't replicate what the user wrote, particularly the tuple element names, then they're subverting the will of the developer and making their generated code much harder to consume.

This applies when the source generator produces an implementing declaration: a well-behaved generator should just copy the signature of the user-authored defining declaration. But in the case where the generator produces a defining declaration and the user authors an implementing declaration, the user has to align with a generator that they likely do not control. Still, it's hard to picture a situation where a user is inconvenienced by being unable to substitute 'dynamic' for 'object' or 'IntPtr' for 'nint' or vice versa.

Probably the main reason to allow such differences for return types is probably that 'object'/'dynamic' etc. differences are currently allowed for parameters, so to only disallow them for returns is inconsistent.

Shouldn't the difference in nullability be an error? Perhaps we could allow differences when one annotated partial is defined in a #nullable enable context and the other unannotated partial is defined in #nullable disable but allowing other cases seems incorrect to me. #Closed

The partial method checks on parameters allow for "safe" nullable variance.

partial void M(string s);
partial void M(string? s) { } // ok

partial void M(string? s);
partial void M(string s) { } // warning

partial void M(IEnumerable<string> s);
partial void M(IEnumerable<string?> s) { } // ok

partial void M(IEnumerable<string?> s);
partial void M(IEnumerable<string> s) { } // warning

I chose an analogous behavior on return types in order to be consistent with this.

Please consider testing all pairs of:

#nullable enable
partial object F();
partial object? F();
#nullable disable
partial object F();
partial object? F();

In reply to: 441084726 [](ancestors = 441084726)

I'm not sure I understand this suggestion. There are only defining declarations in this sample. #Closed

Sorry for the confusion. I was suggesting pairs of those signatures where one is a partial method declaration and the other is a partial method implementation.

In reply to: 441120309 [](ancestors = 441120309)

} [](start = 8, length = 1)

Is there a reason to support differences in dynamic / object, or nint / System.IntPtr?

I think we will try to answer this question here: #45128 (comment)

Not blocking but I think a lot of these tests wheree there is no difference could be more succinctly expressed as a [Theory] with the equivalent types expressed as arguments.

} [](start = 0, length = 1)

I think we should report an error for differences in native integers and underlying types.

Filed a follow-up issue.

definition.ConstructIfGeneric(implementation.TypeArgumentsWithAnnotations) [](start = 16, length = 74)

constructedDefinition? #Closed

= definition.ConstructIfGeneric(...)

error [](start = 27, length = 5)

Consider updating comments so it's clear this is a "warning". #Closed

funny, I guess I created these baselines before filling out some of the boilerplate to make the diagnostic a warning.

If the types are distinct, why not report that, regardless of whether the types contain errors?

This is based on patterns I saw in CheckOverrideMember, it seemed like reducing the cascading errors was best.

it seemed like reducing the cascading errors was best.

Ok, but it doesn't seem like a cascading error to me though because the types are different.

In reply to: 442526990 [](ancestors = 442526990)

Both partial method declarations must return by reference or neither may return by reference. [](start = 41, length = 93)

The message seems a little misleading since both are returning by reference. #Closed

True however I struggled with coming up with a higher quality message. Maybe "Both partial method declarations must have equivalent 'ref' or 'ref readonly' modifiers"..? #Closed

Maybe "Both partial method declarations must have equivalent 'ref' or 'ref readonly' modifiers"

Sounds good. Perhaps strengthen it to "... must have matching 'ref' or 'ref readonly' modifiers."

In reply to: 442527921 [](ancestors = 442527921)