| Age | Commit message (Collapse) | Author |
|---|
|
This scans compiler-generated methods together as a group when marking
the corresponding user code.
There are two scans that we do:
1. An initial scan to determine whether we need to run the dataflow
scanner. It is also what marks static dependencies of the method IL.
2. The full dataflow scan (only if the initial scan says we need
to). This produces warnings and marks reflection dependencies.
Both scans are now done for the group of compiler-generated methods
when marking the user code. For now, we only do the dataflow scan once
per method, but in a later change, we will need to allow re-scanning
compiler-generated callees (as part of the full scan for the
corresponding user code) to properly track captured state.
When compiler-generated code is accessed via reflection, we now don't
do a dataflow scan because we don't have the context that might be
captured from user code. This also means that reflection-dependencies
of the compiler-generated code aren't kept, and dataflow warnings
aren't produced, unless the code is reached through the corresponding
user method. To guard against this, there are new warnings on
reflection access to compiler-generated code. The reflection access
warnings are only for compiler-generated code which would normally
require the reflection method body scanner. This is a heuristic meant
to conservatively approximate "this compiler-generated code would
produce dataflow warnings if invoked by reflection with an unknown
context".
|
|
* Change FlowAnnotations to a ref type (class) - in AOT and linker it stores non-trivial amount of state (caches) and thus should be shared (ideally single instance). Struct is not very practical for such usage.
* Explicitly mark all sources in the shared project as `#nullable enable` since Native AOT doesn't enable nullable project wide yet.
* Add several new intrinsics which are specific to AOT - considered separating these out, but it's probably not worth it. It's likely the analyzer should eventually react to these as well, so linker will be the only one not using them.
* Added one more test for an interesting case of RUC suppressing warnings in attribute processing.
|
|
First one is for the case where Derived type is annotated and has a virtual RUC method (and the base type has that same method with RUC as well). The warning should be generated for the base RUC method.
Second one is for type hierarchy marking of non-public members on base types when the derived type has the annotation. This is a test for https://github.com/dotnet/linker/issues/2813
|
|
* Don't crash analyzer for ref-return assignment
* Same for flow capture references
* Adjust tests after merge
* PR feedback
Add issue links to tests
|
|
Warn instead of throwing on unhandled reference store
I scoped the exception down to the case of a typed value which is of interesting type. Currently we don't think this can every happen and it's basically the only case where there's a potential for analysis hole.
Added a test case which actually reproes the failure from the runtime - not exactly (it seems the compiler in runtime is slightly different or something else is going on and the exact same code from runtime produces different IL in the linker tests), but modified so that it does hit the exception in unchanged code.
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
|
|
|
|
Use RUC warnings to validate correct marking caused by attribute data flow.
The analyzer side of these tests doesn't work yet as they use string->type marking and the analyzer is missing a type name parser for now.
|
|
Type parameters for Roslyn-generated types are all alpha renames, so
there should be a unique original type parameter to map back to, which
can be used to get the original user attributes applied to the type
parameter. This change tries to build a map back from types to
constructor calls, which should map to the original type parameter annotations.
Only handles async and iterator state machines fully right now. Full support for
lambdas and local functions is left as future work.
|
|
Fixes #2799
|
|
|
|
|
|
Make sure that MethodImpls point to an interface that the type
implements or a base type that is inherited from.
|
|
[main] Update dependencies from dotnet/arcade
- Update the linker repo to .NET 7
Some small refactoring of the build files to avoid having to update multiple places with the new TFM.
Note that not all can be updated as they are used verbatim in the NuGet package, so can't rely on repo-only properties.
Also currently I left the source code to repeat these. Eventually we might investigate generating `.cs` files in the msbuild to "inject" the TFM and other constants from the MSBuild to the compiled code.
For the Roslyn tests, I hardcoded a new 7.0.0-preview.2 ref package reference, but this feels really weird - note that so far we've been testing against 6.0.0-preview.5 version. Ideally there would be some way to deduce the ref package version from the currently used SDK, and use that in the tests.
The formatting changes are induced by running `lint`. I assume this is because of the SDK version change as well, but I don't know for sure.
- Merge branch 'darc-main-3a65fa7f-262c-4578-97fd-670249162fc8' of https://github.com/dotnet/linker into darc-main-3a65fa7f-262c-4578-97fd-670249162fc8
- Merge remote-tracking branch 'mono/main' into darc-main-3a65fa7f-262c-4578-97fd-670249162fc8
- Update to preview 3 SDK
- Update to .NET 7 Preview 4 which should have the necessary changes.
- Formatting
|
|
Match analyzer and linker methods that handle Requires to be more similar according to their behavior
Delete non existent ILLink.xsd from projitems
Fixup IL3052 and add SharedStrings from NativeAOT
|
|
* Revert "Trim static interfaces (#2741)"
This reverts commit a073a68561a7f45a2dba0976083ee3e9873bf423.
* Revert "Don't remove MethodImpl if overridden method is not in a link assembly (#2771)"
This reverts commit 44610681fb096cc2b38a18689e954af24049d390.
* Revert "Fix NullReferenceException when sweeping unused static interface (#2783)"
This reverts commit 00e9a154efeb6369b1345bdafeebb686c0163841.
|
|
* Fix NullReferenceException when sweeping unused static interface
The case here is if the static interface itself is actually used, but the method on it is not and the implementation of that method is directly referecned. In that case we should remove the iface method, but keep everything else.
This can cause (depends on order) a NRE in the sweep step since the removed iface method is still in the list of overrides for the implementation method.
* Formatting
|
|
Duplicate attributes were producing warnings on every callsite in
the linker to GetLinkerAttributes, which makes the number of
warnings depend on the number of callsites in user code.
Deduplicate these by producing the warning only once, when
building the linker attributes cache.
Fixes one of the remaining issues encountered while adding trim
analysis patterns in the linker.
Make `TryGetLinkerAttribute` fail if there are multiple attributes.
|
|
Added a test based on array analysis which uses integers as indeces to the array.
|
|
This means the value of the fields is tracked as a const value instead of a field reference.
This is to support some additional code patterns as well as align the behavior with the linker. Compiler will inline the const fields into the IL effectively removing the field refernce in these cases and linker only sees the constant. So ideally the analyzer should have a similar behavior.
|
|
Before removing overrides, we should check if the overridden method is
in an ignore scope.
|
|
This shared pretty much all of the remaining intrinsics except for `object.GetType` (type hierarchy marking).
The linker move is straightforward and the only functional changes are:
* `Type.GetType()` with unsupported case insensitive search will not produce warnings from its return value
* Removed intrinsics handling of `Activator.CreateInstance<T>` - it's not needed, annotations do the same thing, and avoid generating duplicate warnings. This fixes: https://github.com/dotnet/linker/issues/1483
In the analyzer there are two behavioral differences from linker:
* Searching for assembly name + type name is completely disabled (no warnings, ignored) - see the comment added, but there's no good way to do it in most cases and probably not common. We can improve if really necessary.
* `Type.GetType` doesn't actually resolve any types since we don't have a type resolver implementation in the analyzer. This change makes all the rest work as appropriate, so once we do have a type resolver, it should all just light up.
I added test to cover at least basic type resolver cases in a way that it shows that analyzer doesn't handle it.
Other small things:
* Analyzer's ability to handle boolean constants
* Minor cleanup
|
|
|
|
If a member creates a requirement ask the override/virtual member if it fulfills the requirement and only then warns.
Fixes #2763
|
|
The analyzer produces warning in this case which the linker doesn't.
Test for bug https://github.com/dotnet/linker/issues/2763
|
|
Moves the intrinsic handling of `MakeGenericType`, `MakeGenericMethod` and `Expression.Call` into the shared code.
Other changes:
* Some small refactorings on the shared type system and value system.
* Expose generic parameters on type system proxies
* Fix a bug in analyzer when we're recording patterns, the values must be cloned (as they can mutate during analysis after the record is taken)
* Fix some problems in Nullable<T> handling in MakeGenericType - specifically if we don't know what the T is going to be, we now return just Nullable<> known type (but unknown T).
* Add couple new tests - update existing ones (mainly due to limitation in the analyzer)
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
Some intrinsics require generic instantiation handling. Currently the shared code doesn't have the abitlity to do this, so we leave it as a special case and analyzer/linker have special code to handle it.
In the analyzer this code was missing from the diagnsotic reporting part and thus the intrinsic was handled as a normal method call - which caused problems with TypeDelegator.
Added tests for TypeDelegator which cover these issues.
|
|
Add mapping from WellKnownType to Cecil's MetadataType for quicker checking in IsTypeOf.
Add wrapper method for BCL.FindPredfinedType that takes a WellKnownType.
* Use WellKnownType enum in more places
* Use DisplayName for Namespace comparisons
* Use switch expressions instead of statements
* Use OriginalDefinition.SpecialType for WellKnownType comparisons
* Add MetadataType.Void and delete Ananlyzer WellKnownTypeExtensions
|
|
Moving some simple method handle related intrinsics and unifying the values related to it.
|
|
|
|
Don't mark static interface methods when called on a concrete type, and removed the MethodImpl / Override if the interface method is kept.
Co-authored-by: vitek-karas <vitek.karas@microsoft.com>
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
|
|
|
|
* Support assignment to flow capture references
This uses code copied from Roslyn to detect whether a flow capture is used as an r-value or l-value (or both).
L-value captures are tracked in a separate dictionary which is used to look up captured values when assigning to
l-value capture references. The capture references are not first-class dataflow values under SingleValue, in
order to keep the general dataflow logic separated from the trim-analysis-specific values.
|
|
* Move parameter annotation checks to shared code for analyzer
Add GetMethodThisParameterValue
Make GetParameterAnnotations return a nonnull
DynamicallyAccessedMemberTypes
* Use the shared code for parameter annotation checks in the linker
* Remove redundant handling from analyzer
* Use ReturnsVoid() extension method
* Fall back to annotations for unimplemented intrinsics in analyzer
|
|
- The analyzer now has a logic that handles array blocks independently when an if statement is used, see the following example code:
Type[] arr = new Type[] { null };
if (i == 1)
arr[0] = GetMethods ();
else
arr[i] = GetFields ();
arr[0].RequiresAll ();
Gets translated to a structure similar to:
------[B1]------
-----/----\-----
--[B2]----[B3]--
-----\----/-----
------[B4]------
* The first block (B1) contains the array initialization
* Given that there could be different values depending on the result of the if statement, block 2 (B2) and block 3 (B3) handle the different results. Notice that they both start with a copy of the information on B1 which is their predecessor. B2 and B3 do not interact between them and generate their output based on merging the value of B1 and their own generated value.
* B2 executes GetMethods(); which has the return value PublicMethods, then we merge the values in index 0 which are: empty array from B1 and the return value in B2 (PublicMethods)
* B3 executes GetFields(); which has the return value PublicFields, then tries to access the array but the index is unknown so we return UnknownValue
* Block 4 (B4) executes RequiresAll(); in order to verify if arr[0] fulfills the requirements, we read the different values in the array index 0, meaning the result from B2 and B3. At that moment we attempt to merge the result from the different arrays, we verify that one of the results is unknown and therefore we cannot know if arr[0] fulfills the requirements. Generating a single warning stating that the value is unknown.
- Add tests
|
|
* Add support for RequiresDynamicCode on both RequiresDynamicCodeAnalyzer
and DynamicallyAccessedMembersAnalyzer
Add attribute to tests
Change RequiresUnreferencedCodeUtils to RequiresUtils so it can handle
RequiresDynamicCode too
* Remove the interaction between DAM and RDC for now in tests and code
lint
Add generated test cases
* PR feedback
* Remove RequiresDynamicCode from utils since is not needed by any other
analyzer outside the RequiresDynamicCodeAnalyzer
Remove instance of RequiresDynamicCode in
DynamicallyAccessedMembersAnalyzer
* More references to RequiresUnreferencedCodeUtils
Co-authored-by: Tlakollo <tlcejava@microsoft.com>
|
|
This will make sure that we keep all interfaces and all interface method implementations on such type.
* Change the solution to rely on the optimization setting only
* wip
* Add implicit interface implementation case
* Edit tests to remove issue-specific code and names
* Mark members of CollectedType as kept
* Add private interface test and simplify external interface example
* Replace early exit for non-interfaces
* License headers and use IsMethodNeededByTypeDueToPreservedScope instead of Interface specific version
* Add check for static methods before skipping virtual marking
Check for optimization before skipping marking interface methods for
PreservedScope
Add doc comments
* Add more clarifying comments, move static method check, rename method
Rename IsVirtualNeededByInstantiatedTypeDueToPreservedScope to IsOverrideNeededByInstantiatedTypeDueToPreservedScope
Added more comments describing purpose of methods
Moved the static interface method check to the method that is called on all types regardless of instantiation
* Renames and comment cleanup + tests
This doesn't change product behavior, only renamed methods and improved comments.
Added new tests for the RootLibrary:
- Added a dependency to an "copy" assembly (mainly because I can define a static interface in it)
- Added more combinations to the interfaces/classes in the test
- Since this uses static interface methods I had to enable "preview" language features for the test project and for the test infra.
* More tests for interface behavior
Co-authored-by: vitek-karas <vitek.karas@microsoft.com>
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
|
|
Add check for cref in generic parameter dataflow to avoid failing with a nullreference exception
Refactor code to reuse the while loop for both INamedType and Method symbols
Add tests
Fixes #2724
|
|
(#2556)
|
|
The main change is sharing of the GetConstructor intrinsic and related tests.
To make those tests work this also implements Type.EmptyTypes and String.Empty and Array.Empty intrinsics.
These are all treated as special cases:
* Type.EmptyTypes and String.Empty are actually field accesses, so we don't have infra to share these yet, no need to add one for now
* Array.Empty is a generic method where we need to know the generic instantiation. The current sharing doesn't propagate this information
so this forced the implementation to NOT be shared but instead done as a fallback which the shared one doesn't handle.
The rest of the changes are all about fixing how the shared intrinsic handling propagates return value from intrinsics.
Lot of small subtle bugs around null or emtpy inputs and forgetting to add return value and so on. The main changes are in GetMethod and GetNestedType intrinsics which had lot of issues.
Added tests for all of the corner cases.
NestedTypesUsedViaReflection test has been reworked as previously it didn't work correctly. Lot of the [Kept] attributes were fulfilled by some of the negative tests and thus the positive tests didn't have coverage (for example calling GetNestedType(unknownName) will mark all nested types on the type, so a subsequent call to GetNestedType("knownname") will not have an observable effect).
|
|
|
|
* Add tests for nested functions called from nested state machines
The existing algorithm to recursively discover calls to local functions
from a user method was incorrect for the case where a local function
was called from a state machine local function's MoveNext method.
These methods were being treated as roots for the discovery, as if they
were user code, which caused a failure downstream when we assumed that
each local function belongs to a unique user method - since a local function
might be called from a user method _and_ a nested state machine local function.
The existing behavior also had the issue that a local function state machine's
attributes could suppress warnings from nested local functions, going against the
behavior for normal nested functions in the linker.
* Don't fail on multiple user methods for same nested function
* Correctly handle iterator local functions calling nested functions
This fixes the issues shown in the tests by letting the state machine
types participate in the call graph discovery instead of being considered
as roots. Now any local functions called from state machine local functions
will be associated with the user method, not with the state machine MoveNext
method.
* Fix test for analyzer
* Update src/linker/Linker/CompilerGeneratedState.cs
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
* Adjust comments and docs
* Fix docs
Co-authored-by: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
|
|
of the file (#2469)
Some of the files that hold a license from a third party now have the two licenses as header files
Linker now uses the sdk format of license header instead of the runtime one
Added having the license header as warning in the editor config
Added THIRD-PARTY-NOTICES.TXT file to the repo root
|
|
This adds support for RequiresUnreferencedCode and UnconditionalSuppressMessage on lambdas and local functions, relying on heuristics and knowledge of compiler implementation details to detect lambdas and local functions.
This approach scans the code for IL references to lambdas and local functions, which has some limitations.
- Unused local functions aren't referenced by the containing method, so warnings from these will not be suppressed by suppressions on the containing method. Lambdas don't seem to have this problem, because they contain a reference to the generated method as part of the delegate conversion.
- The IL doesn't in general contain enough information to determine the nesting of the scopes of lambdas and local functions, so we make no attempt to do this. We only try to determine to which user method a lambda or local function belongs. So suppressions on a lambda or local function will not silence warnings from a nested lambda or local function in the same scope.
This also adds warnings for reflection access to compiler-generated state machine members, and to lambdas or local functions. For these, the analyzer makes no attempt to determine what the actual IL corresponding to the user code will be, so it produces fewer warnings. The linker will warn for what is actually in IL.
|
|
* Don't let RUC on cctor silence warnings
|
|
* Fix array dataflow test for passing ref to element
|
|
There seems to be a race condition on Cecil while trying to get GenericParameters, if a multithreaded test asks simultaneously about GenericParameters on the same type Cecil answers could vary creating test failures.
Fixes #2693
|
|
* Add test which mimics what is done in Type.ImplementInterface helper in runtime
There's nothing new in this test, but it's better to have coverage for the pattern as it's used in runtime.
* Add tests for properties
* Simplify
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
|
|
|
|
|
|
Add intrinsic support for Nullable.GetUnderlyingType and support for MakeGenericType with Nullables
Adds ArrayCreationOperation visitors to create ArrayValue's in the analyzer, and adds start of dataflow analysis for array values.
Adds tests to validate dataflow in Arrays.
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
|
