Fix wrong symbol comparisons #65430
Fix wrong symbol comparisons #65430
Conversation
ghost
added
the
Community
| @@ -1090,7 +1090,7 @@ private static bool IsInaccessibleBecauseOfConstruction(NamedTypeSymbol type, Na | |||
| NamedTypeSymbol containingType; | |||
| if (containingTypes.TryGetValue(contextBaseType.OriginalDefinition, out containingType)) | |||
| { | |||
| return !TypeSymbol.Equals(containingType, contextBaseType, TypeCompareKind.ConsiderEverything2); | |||
| return !TypeSymbol.Equals(containingType, contextBaseType, TypeCompareKind.AllNullableIgnoreOptions); | |||
There was a problem hiding this comment.
It is not obvious to me what we are trying to detect here. And we need to understand that before picking the TypeCompareKind. For example, should dynamic vs. object make a difference here? If you have an understanding of our intent here, please share it. Otherwise, we need to get this understanding.
There was a problem hiding this comment.
@AlekseyTs I don't have too much info here. I thought that we might not be caring about object vs dynamic since they are both System.Object in the resulting IL. I think CLS compliance is more about "is this assembly (resulting IL) compliant with some rules?" though I'm not sure if there might be something in these rules that might be concerned with the extra "DynamicAttribute" produced.
Revisiting the behavior when nullable is disabled:
#nullable disable
[assembly: System.CLSCompliant(true)]
public class Base<T>
{
protected class Nested { }
}
public class C<T> : Base<object>
{
protected void M(Base<dynamic>.Nested c) // CLS compliance warning
{
}
}I haven't confirmed yet, but I think AllIgnoreOptions might affect this scenario and remove the warning.
I guess we could use AllNullableIgnoreOptions. I know that this won't be revisited since it's no longer ConsiderEverything2, but I also think ConsiderEverything2 was introduced specifically when doing nullability work. So AllNullableIgnoreOptions should just be good.
What do you think @AlekseyTs ?
There was a problem hiding this comment.
What do you think ?
- I would start with understanding the rule that we are trying to implement. What does it say exactly? Let's find this out.
- I would try to figure out whether tuple names and dynamic differences matter for the purpose of the rule. It doesn't look like other options should matter, or corresponding differences could be detected by native compilers. But I could be wrong.
- I would explicitly include options that we care about rather than using
AllNullableIgnoreOptions. - The legacy native compiler supported dynamic. It would be good to check its behavior around dynamic vs. object difference.
There was a problem hiding this comment.
- I would start with understanding the rule that we are trying to implement. What does it say exactly? Let's find this out.
@AlekseyTs Where can I find the whole rules spec'ed precisely? The rule intended to be implemented here per the doc comment is "Rule 46":
But I don't know where can I see what the "rule 46" says exactly. The brief about the rule in the comment is that a constructed generic type C<type1> can't access protected members of a constructed generic type C<type2>. I think C<object> and C<dynamic> are both the exact same type from the point of view of another language consuming this assembly (they are both C<System.Object>). It doesn't seem like the attribute on the type parameter will matter for the ability to access members in other languages.
The legacy native compiler supported dynamic. It would be good to check its behavior around dynamic vs. object difference.
I'll see if I can download VS 2013 and test that. Is there an easier way to get the native compiler other than using VS 2013?
There was a problem hiding this comment.
Ok. I am comfortable going with AllIgnoreOptions here. Assuming legacy compiler didn't care about object/dynamic difference. Let's confirm that.
There was a problem hiding this comment.
@MadsTorgersen, @dotnet/roslyn-compiler
I am comfortable going with
AllIgnoreOptionshere. Assuming legacy compiler didn't care about object/dynamic difference. Let's confirm that
It looks like legacy compiler cares about object/dynamic difference.
[assembly: System.CLSCompliant(true)]
public class C<T>
{
protected class N { }
}
public class D : C<object>
{
protected void M4(C<dynamic>.N n) { }
}
Observed:
(10,23): warning CS3001: Argument type 'C<dynamic>.N' is not CLS-compliant
Should we preserve this behavior?
There was a problem hiding this comment.
CLS compliance is not considered by any modern framework, and is broken in a number of ways today. Preserving that warning does not concern me.
There was a problem hiding this comment.
@Youssef1313 We discussed this issue internally and reached consensus that it would be fine dropping this warning. Let's add test for this scenario, add test suggested at #65430 (comment) and move this PR forward.
There was a problem hiding this comment.
Sorry for missing this for long. I added the tests.
...Compilers/CSharp/Portable/DocumentationComments/DocumentationCommentIDVisitor.PartVisitor.cs
Outdated
Show resolved
Hide resolved
...Compilers/CSharp/Portable/DocumentationComments/DocumentationCommentIDVisitor.PartVisitor.cs
Outdated
Show resolved
Hide resolved
|
Done with review pass (commit 1) |
| var source = @" | ||
| internal class Test<T> : ICloneable<Test<T>> | ||
| { | ||
| public Test<T> Clone() => new(); |
|
Done with review pass (commit 3) |
|
@dotnet/roslyn-compiler For the second review. |
|
Thanks @Youssef1313 ! |
Fixes #62368
Fixes #58966
Fixes #65428
cc @333fred @jcouv @AlekseyTs
Keeping draft until the behavior of
ConsiderEverything(and the fact that we createConstructedNamedTypeSymbolunder#nullable enable, andSourceNamedTypeSymbolotherwise) is confirmed as a correct behavior. Otherwise, the fix will be much more involving.