[reflection] Convert internal GetCustomAttributes calls to Attribute[] #18176
Conversation
CoffeeFlux
requested review from
akoeplinger,
EgorBo,
marek-safar,
steveisok and
vargaz
as code owners
|
@monojenkins build failed |
|
Failures are relevant, though they're somewhat surprising. I'll see what's up next week. |
|
The latest commit probably merits another look. I found we were inconsistently using the type checks, since they existed only in Attribute (and that was sometimes bypassed), so I moved them to CustomAttribute and handled the case of passing System.Object (which was the source of the test failures, but should have been caught with the type checking). |
CoffeeFlux
force-pushed
the
container-config-test
branch
from
4a067d4
to
fbc8b94
Compare
CoffeeFlux
requested review from
alexischr,
BrzVlad,
directhex,
kg,
kjpou1,
lateralusX,
lewing,
lewurm,
naricc,
radical,
SamMonoRT and
thaystg
as code owners
|
Oops. |
|
I was going to go line-by-line here but it seems like this is a bad merge, a bunch of xml stuff changed? |
|
Yeah, typo'd a git command. Cleaning up now, sorry for the ping. |
CoffeeFlux
force-pushed
the
container-config-test
branch
from
fbc8b94
to
b6361c8
Compare
|
@monojenkins build failed |
|
|
||
| if (IsUserCattrProvider (obj)) | ||
| attrs = obj.GetCustomAttributes (attributeType, true); | ||
| attrs = (Attribute[]) obj.GetCustomAttributes (attributeType, true); |
There was a problem hiding this comment.
This doesn't seem right. obj here is some user-written code, so it could be returning an object[] legitimately. I think you have to like... try to cast and if it fails go through one by one and copy the elements...
There was a problem hiding this comment.
I think CoreCLR just casts for this as well? https://github.com/dotnet/runtime/blob/master/src/coreclr/src/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs#L23
There was a problem hiding this comment.
Yeah the docs explicitly say it's an array of objects representing attributes, so it doesn't have to be attribute[]
There was a problem hiding this comment.
I guess later on in the file we do the 'copy to a list individually, casting each time' dance, so maybe I do need to do it here as well :(
I'll put together some tests to see what CoreCLR does in practice and attempt to match them or open an issue.
CoffeeFlux
force-pushed
the
container-config-test
branch
from
b6361c8
to
342ab52
Compare
|
@monojenkins build failed |
|
@monojenkins build failed |
mono/mono#18176) * [reflection] Avoid creating object[] in GetCustomAttributesBase * [reflection] Migrate Attribute type checking to CustomAttribute.cs Commit migrated from mono/mono@ff6294d
There's no reason to use object[] internally, and it leads to accidental object[]-typed array creation, which then can't be downcast to Attribute[].
The most important part of the fix was applied to the legacy BCL as well, though not the full set of changes.
Fixes #15173
Fixes #15171