Remove buggy IsPublic method from TypeAttributesExtensions #75081
Conversation
When checking visibility we need to use the VisibilityMask because TypeAttributes is a funky flags enum that has multiple entries with the same value (unlike normal flags enums). See https://learn.microsoft.com/en-us/dotnet/api/system.reflection.typeattributes?view=net-8.0#examples We got lucky because the IsPublic extension method isn't actually used anywhere so this was never hit. The same applies to checking for interface, but TypeAttributes.ClassSemanticsMask is the same value as TypeAttributes.Interface so it happens to work, but it's still better to use the canonical code for clarity.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
dotnet-policy-service
bot
added
the
Community
|
Turns out there was already an old issue about this (#16929) and we just had a couple instances left so I fixed them too. |
akoeplinger
changed the title
|
@dotnet/roslyn-compiler |
|
Presumably, we should be able to observe differences in these scenarios with tests; given that there are no failures here, it seems like we have a test gap and I'd like to see some tests covering it. |
|
Alternatively, if we are fixing internal dead code (never used), then I would prefer removing it rather than changing it. |
|
yeah only the IsPublic change is actually a bug but the method is never used, I can remove it. the other cases only work because the *Mask value is the same as the enum entry we're checking. in theory we could leave it as is but I think it's worthwhile just to not spread a bad pattern. |
src/Compilers/Test/Core/Platform/Custom/MetadataSignatureHelper.cs
Outdated
Show resolved
Hide resolved
|
In talking with @AlekseyTs, we'd prefer to leave most of the code as is, rather than changing it, given that these flags aren't likely to change and they work as is. For the bad |
akoeplinger
changed the title
|
@333fred sure, I updated the PR to just remove the IsPublic method. |
|
@dotnet/roslyn-compiler @AlekseyTs for a second review. |
* upstream/main: (45 commits) Adjust an assert in EmitArgument (dotnet#75067) Switch to new VMR control set (dotnet#75056) Disallow ref assignment to ternary or another ref assignment (dotnet#75076) Move some emit tests from Emit2 to Emit3 to avoid hitting UserString heap limit (dotnet#75091) BindAttributeCore - use proper binder to avoid an attribute binding cycle (dotnet#75060) Remove buggy IsPublic method from TypeAttributesExtensions (dotnet#75081) Include initial filter node when searching for nodes to order modifiers Remove additional Gitter link (dotnet#75086) Remove newlines between test run information sections Log messages for Test Results Fix stack adjustment when emitting stackalloc (dotnet#75042) Lock translation of strings used to demonstrate identifier naming styles docs: Correct SDK version in documentation to match global.json (dotnet#75038) Add a test observing lack of an issue. (dotnet#75057) Fix preview refresh on selection for enum flags checkboxes Semantic snippets: handle case with inline statement snippets before member access expression (dotnet#74966) Configure release/vscode branch for nuget publishing Remove MS.CA.Test.Resources.Proprietary PackageReference (dotnet#75037) Allow suppressing nullability warnings in more ref scenarios (dotnet#74498) Update dependencies from https://github.com/dotnet/arcade build 20240909.6 (dotnet#75040) ...
When checking visibility we need to use the VisibilityMask because TypeAttributes is a funky flags enum that uses values that aren't binary exclusive (unlike normal flags enums) so e.g.
NestedPrivate(0x3) would be interpreted asPublic(0x1). See https://learn.microsoft.com/en-us/dotnet/api/system.reflection.typeattributes?view=net-8.0#examplesWe got lucky because the IsPublic extension method isn't actually used anywhere so this was never hit. Remove the method.
The same applies to checking for interface, but TypeAttributes.ClassSemanticsMask is the same value as TypeAttributes.Interface so it happens to work, but it's still better to use the canonical code for clarity.Also found a couple other *Attributes that were described in #16929 and fixed them too. Those are essentially no-ops too.Fixes #16929
/cc @tmat