Restrict embedded XML

[sbomer]

This restricts the embedded XML to prevent modification of assemblies other than the containing assembly. Specifically:

• Descriptor XML may still mark types in other assemblies since it is purely additive
• Substitution XML may not rewrite methods/fields defined in other assemblies
• Attribute XML may not add attributes to other assemblies. (RemoveAttributeInstances may only be specified by embedded XML in the assembly that defines the attribute type)
• Embedded attribute XML may not use '*' to select all assemblies
• Embedded XML of any kind may leave out the assemby tag, instead specifying types of the containing assembly directly under <linker>
• XML specified on the command-line is unchanged

(I'm separating the XML changes out from the rest of #1666.)

[vitek-karas]

I think we will need a an exceptions to the rule:

• We need to be able to assembly="*" on linker attributes in CoreLib (nullable attributes)
  • I think it's fine for CoreLib to be excluded from these rules - we will just have to pre-load it to make things simple.
  • CoreLib is special anyway - too many dependencies on how runtime works in the linker. It will have to be S.P.CoreLib for ILLinker and mscorlib for mono, but that should be also easy to do.

[sbomer]

We need to be able to assembly="*" on linker attributes in CoreLib (nullable attributes)

Actually it's not entirely clear to me why we need this. Is it to support removing an attribute whose definition is compiled into multiple assemblies? Could we just recommend embedding a duplicate attribute XML into such assemblies?

[marek-safar]

Actually it's not entirely clear to me why we need this.

This is needed for compiler inserted attributes. It's not really viable to insert the same XML into all assemblies produced by C# compiler.

We could try to convert that section of XML into XML file argument descriptor as specializing this case will be a pain, we could be in the situation where SPC is not known till later and to ensure we don't have a hole somewhere which would mark attributes before they we read SPC xml.

[marek-safar]

fullname is now redundant for embedded XML format, should we just remove/ignore it instead?

[sbomer]

I think it's good to warn when it's used incorrectly in the embedded XML, otherwise people might try to embed existing XML and wonder why it doesn't work.

Since the assembly is redundant for embedded XML, I made it so that you can leave out the assembly entirely:

<linker>
  <type fullname="..." />
</linker>

I kept support for <assembly fullname="ThisAssembly>" since I think it will be easier to reuse existing XML this way, but I could force the embedded XML not to name the assembly if you prefer.

[marek-safar]

Right, I think this is more wording issue than anything else. Your "It is invalid to use these embedded XML files to modify other assemblies." seems to suggest this will be error where in reality this is a warning. Maybe we could go with even simple mode and merge 2100 and 2101 under the simple warning that warns that fullname value is ignored for embedded XML files (just suggestion)

[sbomer]

I reworded the warnings - hopefully this is better. I'd like to keep them separate because the wildcard can produce a warning for non-embedded XML too.

[sbomer]

Does the compiler inject attribute type definitions into assemblies, or only attribute instances? This approach still allows specifying RemoveAttributeInstances in embedded XML for the assembly that defines the attribute type, and it will remove instances of the attribute everywhere.

we could be in the situation where SPC is not known till later and to ensure we don't have a hole somewhere which would mark attributes before they we read SPC xml.

The lazy loading change will ensure that attribute XML from the resolved attribute type's assembly is processed before we mark attribute instances.

[vitek-karas]

Yes - Roslyn adds attribute definitions into each assembly it needs it in. That's the problem - that's the reason we have the * for assemblies. The only alternative would be to modify Roslyn to somehow annotate the attribute definition when it generates it, but that's tricky (would require us to introduce the attribute as a real attribute and not just linker internal)

[sbomer]

I see, thank you. So it sounds like there are a few options:

1. for CoreLib to be excluded from these rules - we will just have to pre-load it

   • This keeps the existing XML working and seems like the quickest fix for now, so I'll go ahead with it if there are no objections.

2. We could try to convert that section of XML into XML file argument descriptor

   • This seems more flexible long-term. If they're not defined in corelib, the corelib XML isn't really the right place to remove them - it's more like a C# language feature that the linker needs to understand. I wonder what the versioning story is for these special attributes. I don't know details about them, but presumably this list doesn't version with corelib, otherwise there wouldn't be a need to inject the definitions.