Add RequiresAssemblyFiles CodeFix by tlakollo · Pull Request #2015 · dotnet/linker

tlakollo · 2021-05-05T22:37:37Z

Add RequiresAssemblyFiles CodeFix
Add tests
Fixes #2005
Fixes #1986
Add other fixes that exist in RequiresUnreferencedCode but weren't on RequiresAssemblyFiles

Add tests Fix dotnet#2005 Add other fixes that exist in RequiresUnreferencedCode but werent on RequiresAssemblyFiles

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

src/ILLink.CodeFix/RequiresAssemblyFilesCodeFixProvider.cs

src/ILLink.CodeFix/BaseAttributeCodeFixProvider.cs

src/ILLink.CodeFix/RequiresAssemblyFilesCodeFixProvider.cs

test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs

agocke · 2021-05-06T17:47:05Z

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

 					ImmutableArray<ISymbol> dangerousPatterns)
 				{
+					// Find containing symbol
+					ISymbol? containingSymbol = null;


Isn't this FindAttributableParent?

I don't think they are the same FindAttributableParent search for a Method/Property/Event/Field to put the attribute on, this function only looks for local functions and lambdas to warn and otherwise falls back to operationContext.ContainingSymbol

Lambdas can't have attributes so I'm not clear on what this is supposed to be doing.

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

agocke · 2021-05-06T17:58:40Z

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

+					var fieldAccess = (IFieldReferenceOperation) operationContext.Operation;
+					if (fieldAccess.Field.ContainingType is INamedTypeSymbol { StaticConstructors: var ctors } &&
+						!SymbolEqualityComparer.Default.Equals (operationContext.ContainingSymbol.ContainingType, fieldAccess.Field.ContainingType)) {
+						CheckStaticConstructors (operationContext, ctors);


Why are we only checking for static constructors on field access?

agocke · 2021-05-07T00:03:53Z

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

 					ImmutableArray<ISymbol> dangerousPatterns)
 				{
+					// Find containing symbol
+					ISymbol? containingSymbol = null;


Lambdas can't have attributes so I'm not clear on what this is supposed to be doing.

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

Add resources files PR comments

Use context.Options instead of context

agocke · 2021-05-07T21:06:27Z

src/ILLink.CodeFix/xlf/Resources.cs.xlf

+<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.2" xsi:schemaLocation="urn:oasis:names:tc:xliff:document:1.2 xliff-core-1.2-transitional.xsd">
+  <file datatype="xml" source-language="en" target-language="cs" original="../Resources.resx">
+    <body>
+      <trans-unit id="RequiresAssemblyFilesCodeFixTittle">


We don't have localization so you can probably turn this off (I think there's a property in the analyzer project that's set that does this, something like <EnableXlfLocalization>false

These files should be deleted.

src/ILLink.RoslynAnalyzer/RequiresUnreferencedCodeAnalyzer.cs

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

agocke · 2021-05-07T21:23:40Z

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

-				if (assemblyType != null) {
-					// properties
-					ImmutableArrayOperations.AddIfNotNull (dangerousPatternsBuilder, ImmutableArrayOperations.TryGetSingleSymbol<IPropertySymbol> (assemblyType.GetMembers ("Location")));
+		protected override ImmutableArray<ISymbol> GetDangerousPatterns (Compilation compilation)


What does Pattern mean here? Members? Do we want to use the word Dangerous here?

I could rename it to SpecialIncompatibleMembers

agocke · 2021-05-07T21:24:56Z

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

+
+		protected abstract bool TryGetRequiresAttribute (ISymbol member, out AttributeData? requiresAttribute);
+
+		protected abstract bool ReportDangerousPatternDiagnostic (OperationAnalysisContext operationContext, ImmutableArray<ISymbol> dangerousPatterns, ISymbol member);


Some of these probably deserve doc comments. It's not clear to me what each method is supposed to do.

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

src/ILLink.CodeFix/BaseAttributeCodeFixProvider.cs

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

agocke · 2021-05-12T17:53:57Z

src/ILLink.CodeFix/xlf/Resources.cs.xlf

+<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" version="1.2" xsi:schemaLocation="urn:oasis:names:tc:xliff:document:1.2 xliff-core-1.2-transitional.xsd">
+  <file datatype="xml" source-language="en" target-language="cs" original="../Resources.resx">
+    <body>
+      <trans-unit id="RequiresAssemblyFilesCodeFixTittle">


These files should be deleted.

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

agocke · 2021-05-12T17:59:05Z

test/ILLink.RoslynAnalyzer.Tests/Verifiers/CSharpCodeFixVerifier`2.cs

 	public partial class CSharpCodeFixVerifier<TAnalyzer, TCodeFix>
 		   where TAnalyzer : DiagnosticAnalyzer, new()
-		   where TCodeFix : CodeFixProvider, new()
+		   where TCodeFix : Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider, new()


By inserting a resource file in ILLink.CodeFixProvider I needed to add it as part of the using statements, creating ambiguity on what is a CodeFixProvider

tlakollo · 2021-05-12T18:25:13Z

If we remove the static constructor handling some of the tests in RequiresCapability will fail, but I just noticed that Marek disabled those test for being unreliable

agocke · 2021-05-12T18:56:54Z

Ah, yes, we should re-enable those. What test fails though? Does the linker already check this functionality?

tlakollo · 2021-05-12T19:02:09Z

I'm not sure which test was failing on CI, but if I remove the static constructor code it will fail on 3 of the RequiresCapability tests
TestStaticCctorRequiresUnreferencedCode
TestStaticCtorMarkingIsTriggeredByFieldAccess
TestStaticCtorTriggeredByMethodCall
Those were added around the same time the support was added in the analyzer, @mateoatr made the change to support these scenarios in the linker

agocke · 2021-05-12T19:23:55Z

Awesome -- didn't realize this had already been implemented in the linker. Yeah, then we should keep this and add test coverage in the analyzer tests.

I forgot the linker tests were disabled though, I'll look at that.

Add document comments Re-enable RequiresCapability tests

src/ILLink.CodeFix/Resources.resx

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs

agocke

LGTM!

Nice work, looks like a lot of cleaner sharing between the analyzers now

Add RequiresAssemblyFiles CodeFix Add tests Fix dotnet/linker#2005 Refactor analyzer code. Fixes dotnet/linker#1986 Add resources files to codefixer Commit migrated from dotnet/linker@2add126

Add RequiresAssemblyFiles CodeFix

4a98616

Add tests Fix dotnet#2005 Add other fixes that exist in RequiresUnreferencedCode but werent on RequiresAssemblyFiles

tlakollo requested review from agocke and mateoatr May 5, 2021 22:37

tlakollo self-assigned this May 5, 2021

tlakollo requested a review from marek-safar as a code owner May 5, 2021 22:37

lint

7db4826

mateoatr reviewed May 6, 2021

View reviewed changes

src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs Outdated Show resolved Hide resolved

mateoatr reviewed May 6, 2021

View reviewed changes

src/ILLink.CodeFix/RequiresAssemblyFilesCodeFixProvider.cs Show resolved Hide resolved

mateoatr reviewed May 6, 2021

View reviewed changes

src/ILLink.CodeFix/RequiresAssemblyFilesCodeFixProvider.cs Outdated Show resolved Hide resolved

mateoatr reviewed May 6, 2021

View reviewed changes

src/ILLink.CodeFix/RequiresAssemblyFilesCodeFixProvider.cs Outdated Show resolved Hide resolved

mateoatr reviewed May 6, 2021

View reviewed changes

src/ILLink.CodeFix/BaseAttributeCodeFixProvider.cs Show resolved Hide resolved

agocke reviewed May 6, 2021

View reviewed changes

agocke reviewed May 7, 2021

View reviewed changes

tlakollo added 3 commits May 6, 2021 19:23

Refactor Analyzers Code

032ebe1

Add resources files PR comments

Revert some testing changes

ee72dc0

Use context.Options instead of context

Use more properties in CodeFixer

c15cb8d

agocke reviewed May 7, 2021

View reviewed changes

src/ILLink.CodeFix/BaseAttributeCodeFixProvider.cs Outdated Show resolved Hide resolved

src/ILLink.CodeFix/BaseAttributeCodeFixProvider.cs Outdated Show resolved Hide resolved

agocke reviewed May 7, 2021

View reviewed changes

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved

PR comments

459f356

agocke reviewed May 12, 2021

View reviewed changes

Remove extra resources

2436f79

Add document comments Re-enable RequiresCapability tests

agocke reviewed May 12, 2021

View reviewed changes

src/ILLink.CodeFix/Resources.resx Outdated Show resolved Hide resolved

src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs Outdated Show resolved Hide resolved

PR comments

63adfc7

agocke approved these changes May 12, 2021

View reviewed changes

Change some variable names

7463677

tlakollo merged commit 2add126 into dotnet:main May 12, 2021

tlakollo deleted the RAFCodeFix branch May 12, 2021 23:13

akoeplinger mentioned this pull request May 13, 2021

[main] Update dependencies from mono/linker dotnet/sdk#17593

Merged

Youssef1313 mentioned this pull request Jul 9, 2021

Code fixes for IL-class errors (.NET publish trimming errors) are applied multiple times dotnet/roslyn#54602

Closed


		protected abstract bool TryGetRequiresAttribute (ISymbol member, out AttributeData? requiresAttribute);

		protected abstract bool ReportDangerousPatternDiagnostic (OperationAnalysisContext operationContext, ImmutableArray<ISymbol> dangerousPatterns, ISymbol member);

Conversation

tlakollo commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlakollo commented May 12, 2021

Uh oh!

agocke commented May 12, 2021

Uh oh!

tlakollo commented May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tlakollo commented May 5, 2021 •

edited

Loading

tlakollo commented May 12, 2021 •

edited

Loading