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

Data flow incorrectly handles out and ref parameters #101734

Open
Tracked by #101149
vitek-karas opened this issue Feb 18, 2022 · 2 comments
Open
Tracked by #101149

Data flow incorrectly handles out and ref parameters #101734

vitek-karas opened this issue Feb 18, 2022 · 2 comments
Assignees
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Milestone

Comments

@vitek-karas
Copy link
Member

Out parameters should behave like return values in a way - they get assigned a value from within the method body (this works), and they are "read" by the caller (this doesn't work). For example:

TryGetAnnotatedValue (out typeWithMethods);
typeWithMethods.RequiresPublicFields (); // Should IL2067 - mismatch of annotations

static bool TryGetAnnotatedValue([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] out Type typeWithMethods)

The local variable (or stack slot) is not assigned a value after the call to the method with out parameter.

This also means that this doesn't warn:

Type typeWithMethods = null;
TryGetAnnotatedValue (out typeWithMethods);
typeWithMethods.RequiresPublicFields ();

That's correct, but for a wrong reason - the typeWithMethods is tracked throughout the method with a null literal value, nothing else.

But this will warn, twice:

Type typeWithMethods;
TryGetAnnotatedValue (out typeWithMethods); // IL2062 - the `typeWithMethods` doesn't have a value -> unknown value  we check it against the declared parameter
typeWithMethods.RequiresPublicFields (); // IL2062 - the out call above didn't assign value, so it's still "unknown"

This should not warn at all.

There probably other cases where this is broken. What I can see as two issues:

  • The "input" value for a out parameter should only be checked if it's a parameter/field, so to simplify only if it's a value with annotation. Known (literal, string, type) or unknown values should not check anything and should be accepted as those will be overwritten.
  • After the call with an out parameter, the value passed in should be "assigned" the value of the out parameter - so from that point on, it should be tracked as an annotated value (pointing to the out parameter as its source), regardless of what the value was before the call.

The second problem applies to ref parameters as well - the value of the variable/stackslot after a call with ref parameter should be an annotated value (pointing to the ref parameter), since the ref parameter acts like an out parameter after the call. On the other hand before the call, the value must be checked as normal in parameter.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

@sbomer sbomer added this to the 9.0.0 milestone Apr 30, 2024
@sbomer sbomer added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-System.Reflection untriaged New issue has not been triaged by the area owner labels Apr 30, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

sbomer added a commit that referenced this issue May 1, 2024
And add a testcase to cover IL2068.

This just ensures that this particular testcase runs without
allowMissingWarnings for the analyzer, and adds one more testcase
to track the specific missing warning that was encountered in
#101203 (tracked in
#101734).
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this issue May 9, 2024
And add a testcase to cover IL2068.

This just ensures that this particular testcase runs without
allowMissingWarnings for the analyzer, and adds one more testcase
to track the specific missing warning that was encountered in
dotnet#101203 (tracked in
dotnet#101734).
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this issue May 30, 2024
And add a testcase to cover IL2068.

This just ensures that this particular testcase runs without
allowMissingWarnings for the analyzer, and adds one more testcase
to track the specific missing warning that was encountered in
dotnet#101203 (tracked in
dotnet#101734).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
Status: No status
Development

No branches or pull requests

2 participants