Create a schema for the LinkAttribute XML files #2500

jtschuster · 2022-01-13T05:58:00Z

Adds xml schema for ILLink.LinkAttributes.xml, and a more general schema that encompasses all of ILLink.LinkAttributes.xml, ILLink.Descriptors.xml, and ILLink.Substittutions.xml. In test xml files there are relative paths to the relevant schema to enable linting.

Also fixes where the preserve attribute was sometimes "none" and sometimes "nothing".

"none"

linux

test/Mono.Linker.Tests.Cases/Attributes.Debugger/DebuggerAttributesRemoved.xml

vitek-karas

Looks good - just couple of minor things

src/ILLink.Shared/ILLink.LinkAttributes.xsd

vitek-karas · 2022-01-13T09:42:08Z

src/ILLink.Shared/ILLink.xsd

@@ -0,0 +1,193 @@
+<?xml version="1.0" encoding="utf-8"?>


I would rename the file to ILLink.Descriptor.xsd

I made this schema broad enough to cover ILLink.Substitutions.xml as well as ILLink.Descriptor.xml and ILLink.LinkAttributes.xml, so I don't want to name it just for descriptors, but if we don't like ILLink.xsd, maybe something like ILLink.General.xsd?

Hmm - I didn't see it there - does the ILLink.Attributes.xsd "inherit/include" the ILLink.xsd then?
I would probably call it ILLink.Common.xsd, but ILLink.General.xsd works as well.

What's the purpose of the schema? If we would like to give this to developers so that they have IntelliSense, having the schema suggest invalid values has potential to create harm (if there's no IntelliSense, one is forced to read docs; with IntelliSense, one relies on that for things that look "obvious").

Example: ILLink.Substitutions.xml will not look at name, only accepts signature for method. ILLink.Descriptors on the other hand will accept both name and signature and one can use it to preserve multiple methods (<method name="Foo" signature="System.Int32 Bar()"> will preserve all methods named Foo and the one Bar overload).

This one I started as a form of documentation before I realized how different each xml file is. I think that's a good point, this schema is a little too general to provide any value. I'll remove this file and only do the more specific schema.

tlakollo · 2022-01-13T19:33:10Z

src/ILLink.Tasks/CreateRuntimeRootDescriptorFile.cs

 				} else if ((members.fields == null) && (members.methods == null)) {
 					XmlAttribute preserve = doc.CreateAttribute ("preserve");
-					preserve.Value = "nothing";
+					preserve.Value = "none";


It's correct to have only one between the two, but just be aware that seems like runtime is using the 'nothing' notation since is what it was documented. See file.
The way I see it we either agreed on 'nothing' (and change only some linker tests) or agreed on 'none' and have to change other files outside the linker repo who may have used 'nothing' already

I think the guiding force should be what the linker actually recognizes - if it's both, then we should allow both in the schema as well.

Seems like the code is looking for Nothing and if an invalid value is passed then it defaults to Nothing
https://github.com/dotnet/linker/blob/main/src/linker/Linker.Steps/DescriptorMarker.cs#L137-L146

MichalStrehovsky · 2022-01-13T23:25:52Z

src/ILLink.Shared/ILLink.xsd

@@ -0,0 +1,193 @@
+<?xml version="1.0" encoding="utf-8"?>


What's the purpose of the schema? If we would like to give this to developers so that they have IntelliSense, having the schema suggest invalid values has potential to create harm (if there's no IntelliSense, one is forced to read docs; with IntelliSense, one relies on that for things that look "obvious").

Example: ILLink.Substitutions.xml will not look at name, only accepts signature for method. ILLink.Descriptors on the other hand will accept both name and signature and one can use it to preserve multiple methods (<method name="Foo" signature="System.Int32 Bar()"> will preserve all methods named Foo and the one Bar overload).

MichalStrehovsky · 2022-01-13T23:44:08Z

src/ILLink.Shared/ILLink.xsd

+			<xsd:attribute name="name" type="xsd:string" />
+			<xsd:attribute name="signature" type="xsd:string" />
+			<xsd:attribute name="required" type="xsd:boolean" />
+			<xsd:attribute name="body" type="xsd:string" />


Nit: We could restrict this to the two supported values

MichalStrehovsky · 2022-01-13T23:47:47Z

src/ILLink.Shared/ILLink.xsd

+			<xsd:attribute name="name" type="xsd:string" />
+			<xsd:attribute name="signature" type="xsd:string" />
+			<xsd:attribute name="value" type="xsd:string" />
+			<xsd:attribute name="initialize" type="xsd:string" />


Maybe make this bool?

In the parser, this has the same problem as preserve: value of true means "initialize", value of anything else means "don't initialize", but maybe we can cut that stuff off.

MichalStrehovsky · 2022-01-13T23:50:45Z

src/ILLink.Shared/ILLink.xsd

+	<xsd:element name="resource">
+		<xsd:complexType>
+			<xsd:attribute name="name" use="required" />
+			<xsd:attribute name="action" use="required" />


Maybe add a type that restricts this to remove?

MichalStrehovsky · 2022-01-13T23:53:01Z

src/ILLink.Shared/ILLink.xsd

+				<xsd:element ref="type" />
+			</xsd:choice>
+			<xsd:attribute name="fullname" type="xsd:string" />
+			<xsd:attribute name="name" type="xsd:string" />


Do we support name on type?

The documentation has name supported on nested types, but not on top level.

What does the code do?

Looks like it also allows name on nested types but not on top level.

protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNavigator nav, bool warnOnUnresolvedTypes) in ProcessLinkerXmlBase.cs gets the fullname attribute, but in ProcessType in LinkAttributesParser.cs it will look for name, and doesn't look for fullname

MichalStrehovsky · 2022-01-13T23:54:01Z

src/ILLink.Shared/ILLink.xsd

+			<xsd:attribute name="required" type="xsd:boolean" />
+			<xsd:attribute name="preserve" type="preservevalues" />
+			<xsd:attribute name="feature" type="xsd:string" />
+			<xsd:attribute name="featurevalue" type="xsd:boolean" />


Add featuredefault?

Design proposal for DAM-on-type and RUC interactions (dotnet#2168) Co-authored-by: Sven Boemer <sbomer@gmail.com> Fix analyzer nullref on assembly attribute (dotnet#2534) The analyzer runs for property assignment operations, including those in attributes. To check whether the property assignment should warn, we look for RUC on the containing symbol. However, assembly-level attributes are contained in the global namespace, which has a null containing type. Create a schema for the LinkAttribute XML files (dotnet#2500) Adds xml schema for ILLink.LinkAttributes.xml, and adds relative paths to the schema in xml used in unit tests to enable linting. [main] Update dependencies from dotnet/runtime (dotnet#2539) [main] Update dependencies from dotnet/runtime Title and message resources should be enforced to exist to prevent printing empty messages (dotnet#2538) Currently, missing resources will default to an empty string printing an empty message for the diagnostics. Code should not try to gracefully handle errors. Included in this PR: - DiagnosticString should be able to locate a title and message resource strings otherwise it throws - Add title resource strings for all missing diagnostic Ids Update dependencies from https://github.com/dotnet/runtime build 20220123.5 (dotnet#2542) [main] Update dependencies from dotnet/runtime Update dependencies from https://github.com/dotnet/arcade build 20220121.6 (dotnet#2541) [main] Update dependencies from dotnet/arcade Analyzer warns when Requires... Attribute is on a static constructor (dotnet#2455) Adds warnings in the analyzer for IL2116 (RUC not allowed on static ctor) and adds IL2117 (RequiresDynamicCode not allowed on static ctor) and IL2118 (RequiresAssemblyFiles not allowed on static ctor). Fixes ExpectedWarning not having effects on constructors by adding a Visit override in the compilation tree walker in the test infra. Adds case in GetDisplayName to differentiate .cctor's and .ctor's. Linker doesn't seem to be checking ctors for calls to methods with RUC. This commit removes the check for the expected warnings in the linker until the bug is fixed.

Adds xml schema for ILLink.LinkAttributes.xml, and adds relative paths to the schema in xml used in unit tests to enable linting. Commit migrated from dotnet/linker@3a06cc8

jtschuster added 10 commits January 12, 2022 15:07

Initial Schema work

8cc539b

Finalize schema so all test xml is valid

abac411

Add restrictions on some attributes and change uses of "nothing" to

5f3bdad

"none"

Initial Schema work

79ae2c2

Finalize schema so all test xml is valid

ed7b9e9

Add restrictions on some attributes and change uses of "nothing" to

f837645

"none"

Remove schema reference in generated file expectation

437f5cd

merge with main

3ebd1fb

Adds a more restrictive schema for LinkAttributes and updates paths for

b142870

linux

Update docs to use 'none' instead of 'nothing'

8f200b1

jtschuster requested review from marek-safar and sbomer as code owners January 13, 2022 05:58

jtschuster requested review from agocke, tlakollo and vitek-karas January 13, 2022 05:58

tlakollo reviewed Jan 13, 2022

View reviewed changes

test/Mono.Linker.Tests.Cases/Attributes.Debugger/DebuggerAttributesRemoved.xml Outdated Show resolved Hide resolved

jtschuster added 2 commits January 12, 2022 22:35

remove schema from xml expectation file

6838288

remove namespace from xml schema location

6f5eb8f

vitek-karas approved these changes Jan 13, 2022

View reviewed changes

jtschuster added 2 commits January 13, 2022 09:39

rename 'typechild' and allow multiple nested types

a1c1033

remove <type> option from <linker> in LinkAttributes

c8e9f4b

tlakollo reviewed Jan 13, 2022

View reviewed changes

MichalStrehovsky reviewed Jan 13, 2022

View reviewed changes

agocke force-pushed the main branch from e485816 to b3c58ad Compare January 13, 2022 23:58

jtschuster requested a review from radekdoulik as a code owner January 13, 2022 23:58

agocke force-pushed the main branch from b3c58ad to c4abaf3 Compare January 14, 2022 00:25

jtschuster added 4 commits January 18, 2022 17:11

Remove ILLink.xsd

4c0cb95

Add more restraints on xml schema

f70da3d

Rename "none" to "nothing" in CreateRuntimeRootILLinkDescriptorFile

fe36b38

change docs back to using preserve='nothing'

9637260

Merge branch 'main' into XmlSchema

504e4a2

jtschuster merged commit 3a06cc8 into dotnet:main Jan 22, 2022

jtschuster deleted the XmlSchema branch February 17, 2022 17:25

Create a schema for the LinkAttribute XML files #2500

Create a schema for the LinkAttribute XML files #2500

Uh oh!

Conversation

jtschuster commented Jan 13, 2022

Uh oh!

Uh oh!

vitek-karas left a comment

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!

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!

tlakollo Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlakollo Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

jtschuster Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlakollo Jan 13, 2022 •

edited

Loading

tlakollo Jan 13, 2022 •

edited

Loading

jtschuster Jan 20, 2022 •

edited

Loading