-
Notifications
You must be signed in to change notification settings - Fork 128
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
Fix behavior of intrinsics with empty inputs #2652
Conversation
This applies to basically any intrinsic which returns a value of interest. It's also a bug in both linker and analyzer. In the analyzer it's more common, because analyzer doesn't use I wonder if there's a way to somehow make this a bit more general solution. But it's tricky. |
71a60e4
to
ad184a5
Compare
Thanks for the suggestion - I added tests for null/no-value flowing into intrinsics where it made sense and it caught a few more bugs, including a crash in the linker. :) I left in some warnings that we're currently producing in the linker, so please let me know if you think any of these should be changed:
I also didn't add tests for AppDomain.CreateInstance* which currently only has basic testing: linker/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs Lines 383 to 384 in 45b481f
|
Not really a general solution, but I did at least add an exception here to force all of the shared intrinsics to set a return value (when they have return types): /~https://github.com/dotnet/linker/pull/2652/files#diff-4d295034befb7f16882c6686e3bc9dcc12ce427af6af6cef53ebc0ca3a7562baR186-R187 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all of the new tests!
if (returnValue == null) | ||
returnValue = calledMethod.ReturnsVoid () ? MultiValueLattice.Top : GetMethodReturnValue (calledMethod, returnValueDynamicallyAccessedMemberTypes); | ||
if (returnValue == null && !calledMethod.ReturnsVoid ()) | ||
throw new InvalidOperationException ("No return value set for intrinsic"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how many times we've been struggling with this (and thus the implied lack of test coverage) - I'm a bit nervous making it throw here (if linker throws, there's basically no workaround for trimming the app). Maybe we should assert and fallback to returning the annotated value - that should in theory lead to warnings which people report to us as noise (and we still catch the most glaring holes in our tests if we hit them).
On a related note (as this has been a recurring topic recently) - maybe we should use Debug builds of linker in our repos (runtime, MAUI, ...) when they run their Debug CI legs - that way we get better test coverage of the linker without the need to break app trimming with exceptions (we could basically use asserts more frequently without losing that much data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this as before, because some of the newly shared intrinsics rely on the fall-back. Even though they don't return types, they currently get are tracked as ValueWithDynamicallyAccessedMembers, and then cause warnings in some cases, like MakeGenericMethod. I think these should be changed to track UnknownValue.
- Fix test - Replace exception with an assert
Some of the new shared intrinsics that don't produce type values need to have some tracked value. Presumably this should really be Unknown, but currently they fall back on the shared logic to track a value with annotations (which for these intrinsics will be None).
* Fix behavior of intrinsics with null or empty inputs Add tests for intrinsics receiving null or empty values, and tweak a few intrinsics to avoid warnings for these cases. This fixes some unnecessary warnings, and also fixes a crash in the linker. Some of the new shared intrinsics that don't produce type values need to have some tracked value. Presumably this should really be Unknown, but currently they fall back on the shared logic to track a value with annotations (which for these intrinsics will be None). A few unnecessary warnings are left as-is to avoid changing the linker behavior. Commit migrated from dotnet/linker@6aa9837
The latest analyzer is producing warnings in runtime for this pattern:
/~https://github.com/dotnet/runtime/blob/959a13fa0d1822dc4c7bfc99a9d5e7a0c50d6306/src/libraries/System.Data.Common/src/System/Data/Common/DbProviderFactories.cs#L129
This fixes that by ensuring that
AssemblyQualifiedName
and other similar intrinsics pass through aTopValue
input instead of falling back on the annotations when there is no input. (The analyzer intentionally returnsTopValue
for unimplemented intrinsics, and the idea is that these should not produce any warnings.)