This repository was archived by the owner on Nov 16, 2023. It is now read-only.
Compilation: Fix custom attributes using types defined in the workbook #106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bojanrajkovic
requested review from
abock,
lewing and
sandyarmstrong
as code owners
|
build |
|
This smoked clean on all platforms, and is mergeable, however, it's technically still broken on platforms that patch the assembly name to include the sequence number (Console/Mono, iOS, Android, Mac). I want to do a little more investigation, but I need to dump the assemblies generated by the compilation pass to do this. I suspect this has something to do with the way that types are encoded when passed to custom attributes. |
|
|
||
| // On a Windows host, we can skip patching for Console, .NET Core, and WPF, but | ||
| // must patch for everything else. | ||
| if (hostIsWindows) { |
There was a problem hiding this comment.
if (!ClientApp.SharedInstance.Host.IsMac)
There was a problem hiding this comment.
Why did I write it this way 🤔...probably had a different structure the first time.
Most things work fine with the space in the generated assembly name, but if it is passed to COM anywhere (notably, as a parameter to a custom attribute constructor), a TypeLoadException is encountered because the COM-side doesn't seem to be able to load the type (though the assembly is resolved just fine!). Replace the ' XIS' part of the string with a bear emoji (a roughly equivalent 4 byte sequence).
| @@ -131,7 +131,7 @@ static unsafe void Patch (byte [] target, byte [] magicBytes, PatchHandler patch | |||
| var length = 8; | |||
| while (true) { | |||
| var b = target [i + length]; | |||
| if (b == 0 || b == '.' || b == '>') | |||
| if (b == 0 || b == '.' || b == '>' || b == '+') | |||
added 2 commits
November 27, 2017 13:38
…tion
Part 2 of fixing custom assemblies! When a type defined inside of a submission
is provided to an attribute as a parameter, it's encoded as such:
```
.custom instance void '<monkeyface>0001#0-0'/MyCustomAttribute::.ctor(class
[mscorlib]System.Type) = ( 01 00 10 F0 9F 90 B5 30 30 30 30 23 30 2D 30 2B
46 6F 6F 0 0 )
```
The bytes there are a string bytes: a few header bytes, F0 9F 90 B5 is the
monkey face emoji, 30 30 30 30 is 0000, 2B is a +, and 46 6F 6F are the string
`Foo`, the type name, then two null bytes indicating the end of the string. This
is standard encoding for a nested type name, and classes defined in
submissions *are* nested in a broader class defined for the submission.
Notice that the `0001` indicates that we're in the _second_ submission, but the
type name hasn't been rewritten for the custom attribute. The crux here is that
because of the `+` that is used for nested types, the script compilation patcher
doesn't break early enough, and the matched string doesn't match the full
assembly name. By adding `+` to the compilation patcher's termination markers,
it allows it to find the assembly name correctly in the nested type definition,
and patch it as well.
Adds a regression workbook for custom attributes with type parameters being passed types defined in the current workbook. The output here should print the FQTN of `Foo`.
sandyarmstrong
approved these changes
Nov 27, 2017
abock
approved these changes
Nov 27, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Custom attributes (either from libraries or defined in the workbook itself) would cause the CLR
to throw
TypeLoadException(with an empty type name!) when trying to load the type to createthe custom attribute instance, somewhere deep inside COM bits (I tracked it to CreateCaObject,
which is an internal call into the CLR).
Changing our assembly name prefix to not have any spaces fixed the initial submission (no
TypeLoadException, the correct output from the sample workbook in #104), but subsequentsubmissions would throw
TypeLoadException(this time with a non-bogus type name!) becausethe assembly name had changed.
The 2nd commit in the series fixes that as well, by improving the detection for when the assembly name has ended—the
+used for nested classes wasn't being detected.Adds the workbook from #104 as a regression workbook as well--this is a good test to have on hand,
as the kind of scenario this is likely to get hit in (JSON.Net + custom
JsonConverter) is important.Fixes #104.
To-do