| Age | Commit message (Collapse) | Author |
|---|
|
|
|
Fixes https://github.com/dotnet/linker/issues/2888
|
|
Syncing back updates to the shared code from NativeAOT.
Mostly formatting - runtime has enabled an analyzer which requires all members to have visibility assigned explicitly.
Some cleanup.
|
|
dependency (#3096)
The tests we have in the linker try to ensure that something can only be kept for a certain reason, but it can be hard to ensure it's not being kept for another reason. For example, sometimes we use typeof(TestType) to mark a type, but that also makes it relevant to variant casting, which can cause parts of TestType to be kept for unintended reasons.
This PR creates the KeptByAttribute which accepts a fully qualified string in Cecil format to indicate depenency provider (the thing that is creating the dependency that marks the item with the attribute), as well as a string representing the DependencyKind to specify the "reason" there was a dependency between the two. It also can accept a System.Type, or a System.Type + name of the member to indicate the dependency provider.
|
|
marked (#3098)
* Don't mark an override every time the base is abstract, only if the declaring type is also marked
Adds a condition to ShouldMarkOverrideForBase to exit early if the declaring type of the method is not marked.
|
|
Changes to processing of compiler generated methods lead to a state where we don't call constant prop and branch removal in all cases before we mark instructions of the method. This can lead to overmarking
This change fixes this by making sure that the branch removal executes on the method in all cases before we mark instructions of the method.
The change guarantees that all accesses to Body are after the constant prop/branch removal happened on the method.
This does have one possibly negative impact: the issue described in https://github.com/dotnet/linker/issues/2937 is now consistent and happens always.
Added tests.
Note that there's still a whole in analysis of compiler generated code around state machines, see https://github.com/dotnet/linker/issues/3087
Basically if there's a local function which is going to be removed due to branch removal and if the body of that method contains code which produces a warning due to generic parameter validation, such warning will always be generated even though it's "dead" code and even if it's suppressed via RUC or similar.
In such case the analysis can't figure out to which method the local function belongs (since the call site has been removed).
|
|
Instead of checking every virtual method to see if it should be kept due
to a base method every iteration of the MarkStep pipeline, check each
method only when its relevant state has changed.
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
(#3081)
Don't assume there is only one parameter for set and none for get. Allow DAM on the index parameters of properties.
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
(#3077)
Change analyzer versions such that the repo can be built with .NET 7 RC2 SDK.
- Took the versions from dotnet/runtime.
- Remove CheckAttributeInstantiation method since is no longer necessary
- Remove global attributes since they are no longer necessary
- Workaround the fact that compiler injects System.Runtime.CompilerServices.RefSafetyRulesAttribute into every assembly to describe the language version
Co-authored-by: tlakollo <tlakaelel_axayakatl@outlook.com>
|
|
mentions (#3044)
We were lacking validation for some of the warnings, and so change in behavior of the product wasn't reflected in the tests. This cleans up tests for IL2053 and IL2054 (which are not produced anymore).
|
|
* Remove --keep-facades option
|
|
* Improve warning message for DynamicDependency to include the name of the type along with the name of the member we were looking for.
* Fix tests
|
|
Use new version of MicrosoftCodeAnalysisVersion in linker (Copy from runtime) to fix the dotnet format command
Run the dotnet format command
Reenable Lint check in ci pipelines
|
|
|
|
Allow codefixer to insert a fix in accessor declarations if we target methods
Fix tests that had accessors and were annotating properties/events
Add tests
|
|
instantiation (#3015)
* Fixes a null ref which happens when an XML comment cref contains generic instantiation
In the cref some of the symbols (for example the type argument) are missing containing symbols, which leads to null refs.
In any case, we should not perform any analysis on symbols inside crefs, only on real code.
So this modifies the analyzer to ignore any symbol inside a cref.
Adds a test to validate this.
* Simplify the test
|
|
The motivation of the change is to reduce the number of unnecessary warnings around PInvokes. The change makes the RequiresReflectionMethodBodyScannerForCallSite check for COM to decide whether dataflow analysis is needed for PInvokes.
|
|
|
|
This replaces
https://github.com/dotnet/linker/commit/82c6dc6f82aeb90cfbe509ae07cf539eecb75550
with a different approach. Now SuppressTrimAnalysisWarnings only
suppresses those warnings defined to be part of the "trim analysis"
category, using a separate command-line argument. An exception is 5.0
apps, where the setting continues to suppress specific warnings for
compatibility.
|
|
This includes a sidecar XML file which will suppress new warnings in
the 6.0 framework.
Note that a few of the new warnings for compiler-generated code are
suppressed in 7.0 with attributes on local functions. In the XML I had
to suppress them with an attribute on the user method (since the
signature format doesn't support compiler-generated code).
Also note that this approach only includes suppressions for the core
framework, not OOB assemblies. OOB assemblies or third-party nuget
packages will require a slightly different approach to avoid warnings
about unresolved members referenced in XML, in apps where those
assemblies aren't used. There are no warnings that we need to suppress
in OOB assemblies - only new warnings in already trim-incompatible
code.
This also includes a fix for a varargs method issue I found while
running on the 6.0 framework.
|
|
* Test repro + fix
* fix marked filter for events and properties
* formatting fix
* simplified Suppression constructor
* Add more tests for events and properties
|
|
* Move usage of the semantic model outside of the RegisterCodeFixesAsync method for all CodeFixers since it slows lightbulb creation
* Add GetBestTypeByMetadataName from dotnet/roslyn and use it instead of GetTypeByMetadataName
* Use raw string literals in all codefixers testing files (this makes some tests to require spaces instead of tabs and change line numbers)
* More nit fixes to make test more consistent
* Rename DAMCodeFixProvider to DynamicallyAccessedMembersCodeFixProvider
* Add helper class for codeFixer
|
|
|
|
* Add generic attribute tests
* Update cecil
|
|
Warnings indicating an unnecessary suppression produced by XML are now pointing to the xml location instead of being generated on the member the xml targets.
|
|
* Expand Code Fix support to 23 DynamicallyAccessedMembers trim warnings
* Pass information about where and what attribute needs to be added from the analyzer to the diagnostic so the Code Fix can target a greater number of warnings
* Include additional tests to ensure that Code Fix is provided as expected, and check edge cases
* Multiple annotations and merging of existing annotations is unsupported
|
|
There was a performance problem where we were tracking all
possible returned values throughout the method, since we were
treating them the same as hoisted locals. I'm sure it's still
possible to come up with testcases that show bad performance (for
example assigning many arrays to a hoisted local in the same
iterator), but this should at least fix the immediate issue
encountered in runtime.
|
|
Running dataflow analysis on the added test takes about 20 seconds. Double the number of `yield returns` and it will take several minutes.
In the runtime repo we're running into this on https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTestData.cs (method `SerializableObjects()`). That one has even more `yield returns` and I lost patience waiting for it to finish.
|
|
annotated with Requires (#2973)
|
|
Adds an ILVerifier to check that the IL produced by the linker is valid. Unsafe C# produced unverifiable code, so we skip verification when we pass that flag to the compiler. Also, there are a few warnings that are produced by valid C# with new features like static abstract interface methods and ref fields and ref returns.
In the future, it may be nice to add better error messages with the type, method name, and IL offset that produced the error, and perhaps an [ExpectedILVerifyError] attribute instead of filtering all of a type of error, but those are non-trivial to implement and don't occur in many tests (<10), so I haven't done that yet.
|
|
|
|
Adds support and basic tests for ref fields. The tests ensure that a value written to an ref field is properly annotated, or an address written to a ref field has a properly annotated value, but there are some holes that can be created by assigning the address of a local or over-annotated value to an annotated ref field.
|
|
This prevents invalid IL in situations where removing the last
instruction of a method would result in the last instruction
being a conditional branch. There needs to be some IL in the
not-taken branch for the IL to be valid, so this fixes the issue
by injecting "ldnull; throw" at the end.
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
|
|
Static methods implementing static interface methods are not marked as virtual and were not checked for matching RUC and DAM annotations. This change adds all base methods to the _virtual_methods list to be checked for matching DAM and RUC annotations. This may cause extra/unnecessary warnings for mismatched annotations even a method is removed by the linker.
|
|
Adds test to use ref fields in various ways to confirm that they don't cause the linker to crash.
|
|
|
|
Sync tweaks necessary for the linker/AOT integration
|
|
* Add a test for the `<namespace>` element. There were no tests.
* Expand `CanPreserveTypesUsingRegex` to help highlight how it behaves differently from `<namespace>`
* Add more examples to the docs
|
|
Test for https://github.com/dotnet/linker/issues/2874.
|
|
should warn (#2942)
For correctness linker must be able to validate that generic parameters with new constraint will have their default .ctor preserved. If that can't be guaranteed, it needs to warn.
Added tests for cases around this.
Nullable<> is the only exception to this rule, so changed the code to completely skip any validation for it.
|
|
methods (#2868)
Fixes #2865
Also addresses marking of all static interface methods encompassing the changes from #2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated.
Co-authored-by: Sven Boemer <sbomer@gmail.com>
|
|
(#2925) (#2940)
|
|
Implementation of the feature described in #2891.
|
|
|
|
This is like the existing test, but additionally shows a visible
difference in the warning behavior. When the warning is produced for
generic arguments rather than dataflow, it will show up even if we don't
do dataflow for the local function whose callsite was removed.
When branch removal breaks the link between the local function and
its user method, this can result in warning suppressions that don't
take effect.
We could use the desired warning behavior to decide what the fix
needs to do (whether it should do compiler-generated owning method
detection before vs after constant propagation for the method).
|
|
Serialization discovery is an undocumented quirk that exists for
back-compat in xamarin scenarios. We turned it on by default in .NET
6, but would like to remove such quirks from the default options in
.NET 7.
This changes the (undocumented) command-line option to be
opt-in. Xamarin SDKs will be expected to pass this option by setting
<_ExtraTrimmerArgs>--enable-serialization-discovery</_ExtraTrimmerArgs>.
|
|
See the test for description of what it does. It's basically the linker repro for the case hit in https://github.com/dotnet/runtime/issues/73027#issuecomment-1199199943
|
|
Suppressions can be now placed on properties and events.
|
|
[main] Update dependencies from dotnet/arcade
- Bump to 7.0 preview6 sdk
Fixes the dotnet-format issue.
- Use TryGetValue instead of ContainsKey for Dictionary
Fixes "error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup"
- Seal classes to fix CA1852
- Add 7.0 P6 runtime
- Upgrade SDK to nightly Preview 7 to get a fix for dotnet test bug
|
|
Add tests for some edge cases due to the analyzer not treating
captured parameters as hoisted variables like the linker does.
Currently the analyzer doesn't detect hoisted parameters, and it
will treat them like normal parameters - warning if a parameter
is assigned a value with an annotation that doesn't match the
parameter annotation. The linker will treat captured parameters
the same as other captured variables, tracking all values that
get assigned. So there is a subtle difference in the warning
behavior:
- Linker won't warn on assignment to annotated captured
parameter, but analyzer will.
- Linker will produce dataflow warnings for all values assigned
to a captured parameter, but analyzer will not.
|
