Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AllowNullAttribute, bad reference in CoreUtilities #4624

Closed
wasabii opened this issue Jul 27, 2023 · 7 comments
Closed

AllowNullAttribute, bad reference in CoreUtilities #4624

wasabii opened this issue Jul 27, 2023 · 7 comments

Comments

@wasabii
Copy link

wasabii commented Jul 27, 2023

This is an odd one.

I have a custom process which is trying to parse the assemblies of this project, and generate stubs in another language. As part of that, I've found what might be an error in the assemblies. And I cannot figure out exactly whether it is an error, or some artifact of my process.

Working against the assemblies in the 17.4.0 NuGet package.

Within Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool class, there is a method named set_Instance. It's the setter for the static Instance property. Applied to this method is an "AllowNullAttribute". You can see this in code:

https://github.com/microsoft/vstest/blob/f6dc2a108da42736d0a1bb50372c599799d7c322/src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs#L40C14-L40C14

However, within the assembly within the Nuget package, this attribute is of type [Microsoft.TestPlatform.CoreUtilities]System.Diagnostics.CodeAnalysis.AllowNullAttribute. Notice that it's supposed to be located in the Microsoft.TestPlatform.CoreUtilities assemby. It appears as a TypeRef.

The output of ildsm against TestSessionpool:

.method assembly hidebysig specialname static 
        void  set_Instance(class Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool 'value') cil managed
{
  .param [1]
  .custom instance void [Microsoft.TestPlatform.CoreUtilities]System.Diagnostics.CodeAnalysis.AllowNullAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       9 (0x9)
  .maxstack  8
  IL_0000:  ldarg.0
  IL_0001:  volatile.
  IL_0003:  stsfld     class Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool modreq([netstandard]System.Runtime.CompilerServices.IsVolatile) Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool::s_instance
  IL_0008:  ret
} // end of method TestSessionPool::set_Instance

See the attribute applied.

However, Microsoft.TestPlatform.CoreUtilities from the same NuGet package has no such attribute.

I suspect this has something to do with how ya'll are adding these custom attributes for down level assemblies: https://github.com/microsoft/vstest/blob/main/shared/NullableAttributes.cs

But I'm not sure. Not sure what you could be doing wrong.

Maybe what I'm seeing is that the NuGet package has a mix of assemblies? I don't know. I see that you've got NullableAttributes.cs included into CoreUtilities. There's a bunch of IFDEFs. Is it possible that the version of CoreUtilities being distributed is different than the one the CrossPlatEngine is built against?

@wasabii
Copy link
Author

wasabii commented Jul 27, 2023

It looks like 17.3.1 is correct, though. The Microsoft.TestPlatform.CoreUtilities assembly in netcoreapp2.1 contains the appropriate class.

What I'm seeing is that the addition of the netcoreapp3.1 assemblies is where the issue began. Perhaps it's building against the wrong TFM, and then bundling some other TFM.

@nohwnd
Copy link
Member

nohwnd commented Jul 28, 2023

What exact package are you looking at? I am looking at Microsoft.TestPlatform 17.4.0, and there I can see the type:

// C:\Users\jj\Downloads\microsoft.testplatform.17.4.0\tools\net462\Common7\IDE\Extensions\TestPlatform\Microsoft.TestPlatform.CoreUtilities.dll

image

@wasabii
Copy link
Author

wasabii commented Jul 28, 2023

Check the netcoreapp3.1 version in microsoft.testplatform.testhost\17.4.0\lib\netcoreapp3.1

Not sure what the pacakge without 'testhost' on the end is for, actually. Looks like VS extension stuff?

@nohwnd
Copy link
Member

nohwnd commented Jul 28, 2023

I see it now. Those attributes are emitted into each dll to polyfill the API gaps between modern .NET and old .NET Standard or .NET Framework that does not have them.

The dlls in the package end up being a mix of .net standard and netcoreapp3.1 dlls, meaning that some don't have those types.

Those attributes are used in many places, but are "just" compiler suggestions, which is why it won't fail on runtime.

@wasabii
Copy link
Author

wasabii commented Jul 28, 2023

Yeah. To the runtime. But, from the perspective of a tool, who's job it is to parse those assemblies, and say, convert them to, say, a Java class file, without preknowledge about what they mean, they become a bit of a problem.

Longer story:

I am the maintainer of the IKVM project. We have a test project in our solution that tries to use the vstest framework. Except the tests are written in Java and compiled with javac. To do this, we generate 'stub files' of .NET assemblies: parsing the original assemblies and emitting possibly accessible API into Java class files, that the user's code can build against, before the resulting Java byte code is converted to IL.

From our perspective, Custom Attributes get turned into Annotations. Which the Java compiler has available to it during it's job.

I could black list this particular attribute name, just for this particular project, or something.... but yeah, you see the wider problem. It's the wrong assembly.

This is all automated of course, so it's not like I'm hand encountering this particular assembly, and not like I actually care about this attribute. It's just emitting stubs of everything the user has added to their project, and if the user has added Microsoft.NET.Test.Sdk, then those assemblies are references, and the stubs have to be generated.

Also, I suppose I could change our assemby reading code to skip over attributes who's types are missing and issue a warning or something.

Anyways, figured I'd file the bug. I did finally find a version of VSTest that worked fine: 15.3.0.

@nohwnd
Copy link
Member

nohwnd commented Jul 28, 2023

I am not saying it is not an issue, I am just trying to explain why you are the first person to report it after significant time of shipping this bug, even though the attribute is used in many places.

@nohwnd
Copy link
Member

nohwnd commented Jul 8, 2024

We won't be fixing this, it would require a change to where we emit the attributes, and they are supposed to be compiler suggestions only. Please ignore the errors as you've explained above.

@nohwnd nohwnd closed this as not planned Won't fix, can't repro, duplicate, stale Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants