I didn't quite understand exactly what the problem was. How did the logic below trigger an IVT check and why was that check inappropriate? It seems fair game to ask questions about overriddenMember and leastOverriddenMember since we're referencing them. #Closed

How did the logic below trigger an IVT check and why was that check inappropriate?

Consider test InternalsVisibleToAndStrongNameTests.Issue81820_01 which was failing. The GetLeastOverriddenMember when called on C3.M() performs an IVT check on C1<C2>.M() and the assert fails since C2 is from an "unrelated" assembly (it's not referenced by the assembly that C1 comes from).

It seems fair game to ask questions about overriddenMember and leastOverriddenMember since we're referencing them.

I'm also not sure why this shouldn't be allowed in general (cc @AlekseyTs who added the assert). But in this case it seems fine to narrow the check to OriginalDefinition.

Without doing a deep analysis, I find this change suspicious. I think that the argument to the GetLeastOverriddenMember is wrong. The within type is the type that overrides, not the type where the overridden member is declared. On the other subject, I am surprised that I was not proactively consulted on the issue and instead we had to rely on reviewers to ask questions about the change.

On the other subject, I am surprised that I was not proactively consulted on the issue and instead we had to rely on reviewers to ask questions about the change.

Good point, sorry, I will try to be more proactive next time. I thought I understood what the issue is here until I started thinking more about Julien's question and that's when I summoned you :)

var leastOverriddenMember = overriddenMember.GetLeastOverriddenMember(overriddenMember.ContainingType);

If we are looking for a least overridden member for an override in source, then I think we should start with that override, not from some intermediate override. Then, we wouldn't make a mistake with the context type. The way it looks to me, to implement the current logic, we don't even need the overriddenMember parameter. Then there is also a question whether the check should be performed against the least overridden member, rather than the immediate overridden member, like the other similar methods do. #Closed

Then there is also a question whether the check should be performed against the least overridden member, rather than the immediate overridden member, like the other similar methods do.

You're right, I can simplify this. I originally wanted this to be more similar to Obsolete (which is similar in other ways too; and it checks against least overridden member) but that doesn't seem necessary, wouldn't match the speclet either I guess, and consistency with other modifiers like scoped seems better.

I take that back, as Julien pointed out offline, it makes sense to look at the least overridden member. In case we have unsafe overrides safe overrides unsafe, there should be no error.

Could you perhaps give an example? Also, what does the feature spec explicitly say about the situation? Consider also adding a test that observes the difference.

D.M3() in Member_Method_Override is interesting scenario

Could you perhaps give an example?
Consider also adding a test that observes the difference.

As Julien says, D2.M3() in Member_Method_Override is both an example and a test for this.

Also, what does the feature spec explicitly say about the situation?

The speclet currently only has this simple paragraph for OHI:

It is a memory safety error to add RequiresUnsafe at the member level in any override or implementation of a member that is not requires-unsafe originally, because callers may be using the base
definition and not see any addition of RequiresUnsafe by a derived implementation.

We can probably clarify but I think the intent is clear.

Should we be checking all methods in the chain? Do we assume all code is produced by C# compiler?

Should we be checking all methods in the chain? Do we assume all code is produced by C# compiler?

I guess we could add an open question for this. But it feels like we shouldn't report declaration errors for methods from metadata - if someone marks a method as RequriesUnsafe using a compiler that doesn't support that feature, that's their fault. Similarly we don't check if someone marks a class as RequiresUnsafe in other languages.

overridingMember.ContainingType

this? #Closed

This is inside a static helper.

This is inside a static helper.

Can we assert that overridingMember is definition?

Yes, I think we can.

Actually this won't hold in some complex cases, adding test Member_Method_Implementation_Synthesized to demonstrate one such case.

Can we assert that it is a definition when implementedMember is null? It is important for the situation when GetLeastOverriddenMember is called.

null

Are there call-sites that pass non-null? #Closed

Yes, for interface implementations.