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

ILLink analyzer hole for deconstruction assignments #3158

Open
Tracked by #101149
voroninp opened this issue Dec 11, 2022 · 13 comments
Open
Tracked by #101149

ILLink analyzer hole for deconstruction assignments #3158

voroninp opened this issue Dec 11, 2022 · 13 comments

Comments

@voroninp
Copy link

voroninp commented Dec 11, 2022

I get an error when I'm trying to publish an assembly:

Trim analysis error IL2080: BeforeCommitDomainEventPublisher.d__5.MoveNext(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.All' in call to 'System.Type.InvokeMember(String, BindingFlags, Binder, Object, Object[])'. The field
'System.ValueTuple<T1,T2>.Item1' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

However, analyzer does not emit any warnings when building the project (dotnet build /p:PublishTrimmed=true), yet I have TreatWarningsAsErrors=true.

What can be the reason for it?

Project is targeting .NET, I see ILLink.RoslynAnalyzer among referenced analyzers, severity for IL2080 is set to error.
image

@voroninp
Copy link
Author

voroninp commented Dec 12, 2022

Also I do not get why Linker tries to trim referenced assemblies (Hangfire.AspNetCore for example) even when <TrimMode>partial</TrimMode>.

@voroninp
Copy link
Author

Ok, I know why

@voroninp voroninp reopened this Dec 12, 2022
@voroninp
Copy link
Author

voroninp commented Dec 12, 2022

Well, no I see warnings in Errors window, but again, only after Publish fails. But not warning from Build.

image

@vitek-karas
Copy link
Member

There are some differences between the analyzer and the trimmer - we try to minimize these, but it's not always possible (or easy). But it could also be a bug. The general strategy is:

  • Trimmer tends to play it safe - meaning that if it's not sure, it will raise a warning. It must guarantee that if there are no warnings the program will work.
  • Analyzer is the other way - meaning that if it's not sure, it will not say anything. This is to avoid noise from the analyzer. It might be that this one such case - hard to tell without knowing the details.

Based on the error this is around compiler generated code. That is one area where the analyzer is more likely to "understand" - because it sees the code mostly as written by the user. Unlike the trimmer which sees the compiler generated IL which implements the user's code - sometimes in a rather complex manner. We've been working on this area a lot, but it could be here are still bugs there.

What version of the SDK/trimmer are you using? Ideally the version of the .NET SDK and the target framework for the app (net6.0 or so).

Also - if you have a repro project available, we could look into this some more. Or at least the piece of code where the warning is generated.

@voroninp
Copy link
Author

.NET 7,
SDK 7.0.100

"C:\Program Files\dotnet\sdk\7.0.100\Sdks\Microsoft.NET.ILLink.Tasks\tools\net7.0\illink.dll

@voroninp
Copy link
Author

Do you mind, if I add you as a colaborator to the project?

@vitek-karas
Copy link
Member

No, but I don't see a reason - I won't try to find the right magic to workaround this problem - even if it worked, it would likely break with your next change to any code remotely around it.
I'll try to push the fix through servicing - so that it makes it into some later release of .NET 7 SDK.

@voroninp
Copy link
Author

It's ok, but at least we'll know the reason for this behavior. ;-)

@voroninp
Copy link
Author

Done. If you try to publish API as self contained, it will fail. branch: feature/small-self-contained-container

@vitek-karas
Copy link
Member

Sorry - I was confused between two different issues, so my answer above is a bit weird. Thanks for the repro - I'll take a look.

@vitek-karas
Copy link
Member

I filed a new issue in that repo to discuss the details - please take a look.

@vitek-karas
Copy link
Member

To the specific issue reported in this bug where linker warns but analyzer doesn't. This is a bug in the analyzer. The pattern is actually pretty simple:

			static void DeconstructVariableNoAnnotation ((Type type, object instance) input)
			{
				var (type, instance) = input;  // This is the problematic spot
				type.RequiresPublicMethods (); // Should warn IL2077 - linker does, analyzer doesn't
			}

The problem is the deconstruction statement. Analyzer doesn't recognize it as something which assigns values to locals and so it enters the second line with having two locals type and instance both of which have empty values in the data flow (not unknown, or parameters and so on - empty). So it doesn't warn.

The root problem is that the visitor doesn't handle VisitDeconstructionAssignment. We'll have to implement that - on first look it's not exactly simple - would be nice to have some docs somewhere describing what shape can the nodes around this take.

I'll prepare a simple test and merge that first - so that we have something to test the existing behavior (linker warns, analyzer doesn't).

Workaround (if you can call it that) is to not use deconstructions and instead refer to the items of the constructed type as properties - so the same code can be rewritten like:

			static void DeconstructVariableNoAnnotation ((Type type, object instance) input)
			{
				input.type.RequiresPublicMethods (); // Warns IL2077 in both linker and analyzer
			}

But this is just a pure bug in the analyzer and we need to fix this.

@vitek-karas
Copy link
Member

#3164 adds tests which cover these cases.

vitek-karas added a commit that referenced this issue Jan 3, 2023
This was prompted by #3158
So I just added several tests for various shapes of type deconstructions. To make it symmetric I also added tests for what I call type construction (tuples, anonymous types, records).
tlakollo pushed a commit to tlakollo/runtime that referenced this issue Jan 16, 2023
This was prompted by dotnet/linker#3158
So I just added several tests for various shapes of type deconstructions. To make it symmetric I also added tests for what I call type construction (tuples, anonymous types, records).

Commit migrated from dotnet/linker@72d05e4
@sbomer sbomer changed the title No warning from analyzer, but error when publishing. ILLink analyzer hole for deconstruction assignments Oct 19, 2023
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