RequiresAssemblyFilesAttribute resource manager changes #1790

tlakollo · 2021-01-29T01:41:24Z

PR for #1786
-Change resource file to have a default message and accept arguments
-Add test for RequiresAssemblyFilesAttribute with Url named argument only

-Add test for RequiresAssemblyFilesAttribute with Url named argument only

agocke · 2021-01-29T02:41:21Z

src/ILLink.RoslynAnalyzer/Resources.resx

+    <value>RequiresAssemblyFilesAnalyzer</value>
+  </data>
+  <data name="RequiresAssemblyFilesMessage" xml:space="preserve">
+    <value>Calling '{0}' which has `RequiresAssemblyFilesAttribute` can break functionality when bundling the application code. {1}. {2}</value>


I'm not sure bundling the application code is a well-understood term. The above terminology uses "embedded in a single-file app," which seems understandable for things like Xamarin or Blazor.

agocke · 2021-01-29T02:43:31Z

src/ILLink.RoslynAnalyzer/Resources.resx

  <data name="RequiresAssemblyFilesTitle" xml:space="preserve">
-    <value />
+    <value>RequiresAssemblyFilesAnalyzer</value>


I don't think this is supposed to be the title of the analyzer, it's the title of the warning, which is like a short description of what the warning is supposed to prevent.

Something like, "Avoid calling members marked with RequiresAssemblyFilesAttribute when publishing as a single file"?

agocke · 2021-01-29T02:45:19Z

test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs

@@ -48,7 +48,7 @@ void M()
 	}
 }";
 			return VerifyRequiresAssemblyFilesAnalyzer (TestRequiresAssemblyFieldsOnEvent,


These tests should print out a comment with the message when the test fails. If you include the comment, you can see if it looks right in the message and make the code easier to review.

Are we sure we want the comments? they got deleted from the RequiresUnreferencedCodeAnalyzerTests too. I'm fine with or without them just want to homologate things between the analyzers code.

mateoatr · 2021-01-29T06:28:54Z

src/ILLink.RoslynAnalyzer/Resources.resx

  <data name="RequiresAssemblyFilesTitle" xml:space="preserve">
-    <value />
+    <value>Avoid calling members marked with `RequiresAssemblyFilesAttribute` when publishing as a single-file</value>


Nit: can we change (here and above) the backticks for common apostrophe (') similar to

https://github.com/mono/linker/blob/3c3771c5f0818f9b88387bd2bc1d9e58712e6b31/src/ILLink.RoslynAnalyzer/Resources.resx#L124

Or https://github.com/mono/linker/blob/master/src/linker/Resources/Strings.resx ?

Is calling the right terminology here?. Are we going to add a new message for each bundle like format?

Is calling the right terminology here?

I'm changing the message to match something similar to #1778 (comment)

Are we going to add a new message for each bundle like format?

Since they all will use the same custom attribute I would expect to print the same message independent of the kind of bundle

mateoatr · 2021-01-29T06:34:44Z

src/ILLink.RoslynAnalyzer/Resources.resx

-  <data name="RequiresUnreferencedCodeAnalyzerTitle" xml:space="preserve">
-    <value>RequiresUnreferencedCodeAnalyzer</value>
+  <data name="RequiresUnreferencedCodeTitle" xml:space="preserve">
+    <value>Avoid calling members marked with `RequiresUnreferencedCodeAttribute` when trimming application code</value>


Suggested change

<value>Avoid calling members marked with `RequiresUnreferencedCodeAttribute` when trimming application code</value>

<value>Avoid calling methods marked with 'RequiresUnreferencedCodeAttribute' when trimming application code</value>

Also I'm not sure that something marked with this attribute is something that the user should avoid calling, it's just a method that requires dynamic access, which is dangerous when linking the application (but I really don't have a good suggestion for framing this title).

We could rephrase by saying something like "Members annotated with RequiresUnreferencedCodeAttribute may behave differently or fail if the unreferenced code is removed by trimming"

I think that's a decent description of the problem, but it doesn't really give a call to action, like "don't call this when trimming." I don't have a strong opinion either way, but I think we should decide on what kind of title we want to present.

The context for this resource is that when you're looking at a list of the warnings, this will be the description of what the warning is about. The message is what's actually printed when the warning fires.

In a previous commit, I changed the message to:
Methods marked with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code

mateoatr · 2021-01-29T06:36:44Z

src/ILLink.RoslynAnalyzer/Resources.resx

  </data>
-  <data name="RequiresUnreferencedCodeAnalyzerMessage" xml:space="preserve">
+  <data name="RequiresUnreferencedCodeMessage" xml:space="preserve">
    <value>Calling '{0}' which has `RequiresUnreferencedCodeAttribute` can break functionality when trimming application code. {1}. {2}</value>


Since you're editing things here, I think we should update this message to match the one we produce from within the linker: #1778 (comment)

-Change terminology of analyzers warnings messages to match linker -Change title description of analyzer RequiresUnreferenceCodeTitle

-Change RequireUnreferencedCodeCapability Expected warnings

agocke · 2021-02-02T22:01:03Z

test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs

 						break;
-					expectedMessageContains.RemoveAt (0);
+					var expectedMessages = expectedMessagesContains[1];


It looks like this no longer handles params arrays properly, namely it only looks at the second argument, instead of all the arguments.

…leResourceChange Update error messages Update ExpectedWarning usage

Throw whenever a named argument is passed in ExpectedWarning

agocke · 2021-02-04T19:05:53Z

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

@@ -56,7 +56,7 @@ public static void Main ()
 			TestRequiresInDynamicDependency ();
 		}

-		[ExpectedWarning ("IL2026", "--RequiresWithMessageOnly--")]
+		[ExpectedWarning ("IL2026", new string[] { "'Mono.Linker.Tests.Cases.RequiresCapability.RequiresUnreferencedCodeCapability.RequiresWithMessageOnly()'", "Message for --RequiresWithMessageOnly--." })]


Why add new string[]?

Ok, I changed the code to not support array creation, the code will throw in GetStringFromExpr if it ever tries to be used

agocke

LGTM, although I'd prefer if we settle on one style for params array invocations in ExpectedWarning: either use new string[] everywhere, or don't use it anywhere.

mateoatr

Can we remove the new string[] usage in the tests? Why do we need it? Looks to me that it is enough to just look for the warning code and look for the message that was specified in the Message parameter of the method that we think is the origin of the warning.

mateoatr · 2021-02-04T19:51:06Z

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

@@ -108,7 +108,7 @@ static void TestRequiresOnPropertyGetterAndSetter ()
 			set { }
 		}

-		[ExpectedWarning ("IL2026", new string[] { "'Mono.Linker.Tests.Cases.RequiresCapability.RequiresUnreferencedCodeCapability.RequiresAndCallsOtherRequiresMethods<TPublicMethods>()'", "Message for --RequiresAndCallsOtherRequiresMethods--." })]
+		[ExpectedWarning ("IL2026", "'Mono.Linker.Tests.Cases.RequiresCapability.RequiresUnreferencedCodeCapability.RequiresAndCallsOtherRequiresMethods<TPublicMethods>()'", "Message for --RequiresAndCallsOtherRequiresMethods--.")]


It's enough to only validate the message that's coming from the annotated method. We explicitly removed full text validation previously: #1778 (comment)

My line of thinking was that this is not full-text validation, I only validate the method name and the message which are things that should not change in the message. The linker has code in the result checker to check for the method with the ExpectedWarning attribute. The analyzer on the other hand will match any messages that are within the span, which means that there is a chance for the analyzer to check for messages that were not produced by the method marked with ExpectedWarning. Although in this file, no message is repeated, so using the message is enough. I will change the implementation.

* Change resource file to have a default message and accept arguments * Add test for RequiresAssemblyFilesAttribute with Url named argument only * Add IL3XXX warning codes to the master error code registry * Throw whenever a named argument is passed in ExpectedWarning

…r#1790) * Change resource file to have a default message and accept arguments * Add test for RequiresAssemblyFilesAttribute with Url named argument only * Add IL3XXX warning codes to the master error code registry * Throw whenever a named argument is passed in ExpectedWarning Commit migrated from dotnet/linker@9b71e7b

-Change resource file to have a default message and accept arguments

6d4afbb

-Add test for RequiresAssemblyFilesAttribute with Url named argument only

tlakollo requested a review from agocke January 29, 2021 01:41

tlakollo self-assigned this Jan 29, 2021

tlakollo requested a review from marek-safar as a code owner January 29, 2021 01:41

agocke reviewed Jan 29, 2021

View reviewed changes

Tlakollo added 2 commits January 28, 2021 19:15

-PR feedback

13b2e29

-Reorg comments

2fafeba

mateoatr reviewed Jan 29, 2021

View reviewed changes

Tlakollo added 3 commits January 29, 2021 11:58

-Add IL3XXX warning codes to the master error code registry

4c5977d

-Change terminology of analyzers warnings messages to match linker -Change title description of analyzer RequiresUnreferenceCodeTitle

-Fix new string [] pattern in ExpectedWarning

2a6f22c

-Change RequireUnreferencedCodeCapability Expected warnings

-Delete additional spaces and trailing dots in analyzer message

d37e985

tlakollo requested review from mateoatr and agocke January 30, 2021 00:02

agocke reviewed Feb 2, 2021

View reviewed changes

Tlakollo added 4 commits February 3, 2021 11:32

Merge branch 'master' of https://github.com/mono/linker into SingleFi…

ccd77f3

…leResourceChange Update error messages Update ExpectedWarning usage

Fix test case utils to support different kinds of array strings

879cf6c

Throw whenever a named argument is passed in ExpectedWarning

Revert Analyzer.csproj automatic changes

830a1dc

Change tabs for spaces

9d624a0

agocke reviewed Feb 4, 2021

View reviewed changes

agocke approved these changes Feb 4, 2021

View reviewed changes

mateoatr suggested changes Feb 4, 2021

View reviewed changes

Delete support for new array creation for param array

4c3129d

mateoatr reviewed Feb 4, 2021

View reviewed changes

Only use message in ExpectedWarning

02eaf38

tlakollo requested a review from mateoatr February 4, 2021 22:02

mateoatr approved these changes Feb 4, 2021

View reviewed changes

tlakollo merged commit 9b71e7b into dotnet:master Feb 4, 2021

tlakollo deleted the SingleFileResourceChange branch February 4, 2021 22:07

tlakollo mentioned this pull request Feb 26, 2021

RequiresAssemblyFilesAttribute error code and messages #1786

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RequiresAssemblyFilesAttribute resource manager changes #1790

RequiresAssemblyFilesAttribute resource manager changes #1790

tlakollo commented Jan 29, 2021

agocke Jan 29, 2021

agocke Jan 29, 2021

agocke Jan 29, 2021

tlakollo Jan 29, 2021

mateoatr Jan 29, 2021

marek-safar Jan 29, 2021

tlakollo Jan 29, 2021

mateoatr Jan 29, 2021

agocke Feb 3, 2021

tlakollo Feb 3, 2021

mateoatr Jan 29, 2021

agocke Feb 2, 2021

agocke Feb 4, 2021

tlakollo Feb 4, 2021

agocke left a comment •

edited

Loading

mateoatr left a comment

mateoatr Feb 4, 2021

tlakollo Feb 4, 2021

	<value>Avoid calling members marked with `RequiresUnreferencedCodeAttribute` when trimming application code</value>
	<value>Avoid calling methods marked with 'RequiresUnreferencedCodeAttribute' when trimming application code</value>

RequiresAssemblyFilesAttribute resource manager changes #1790

RequiresAssemblyFilesAttribute resource manager changes #1790

Conversation

tlakollo commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment • edited Loading

Choose a reason for hiding this comment

mateoatr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke left a comment •

edited

Loading