Fix for #270, and framework for testing cross-targeting scenarios #275
Conversation
| @@ -324,25 +324,7 @@ type cenv = | |||
| // [ns] ,name -> ns+name | |||
| // [ns;typeA;typeB],name -> ns+typeA+typeB+name | |||
| let convTypeRefAux (cenv:cenv) (tref:ILTypeRef) = | |||
|
|
|||
| // If an inner nested type's name contains a space, the proper encoding is "\+" on both sides - otherwise, | |||
| // we use "+" | |||
There was a problem hiding this comment.
As far as I can tell from my testing, this comment is just wrong -- I can't find any scenarios where the + is escaped. Indeed, the incorrect escaping needed to be removed in order to get my tests passing. All current tests continue to work fine.
This is now simplified and exactly matches https://github.com/Microsoft/visualfsharp/blob/fsharp4/src/absil/il.fs#L682
There was a problem hiding this comment.
That code had always been very, very suspect to me. Thanks for digging into this so thoroughly
|
For reference, some CLR code that pertains to parsing special chars in reflection-style type names (hooray open source 🎉) Of the designated reserved characters, only |
|
Looks good. Nice work. |
ref #270 - "internal error: null: convTypeRefAux"
More issues with comma escaping, similar to #250. Fix is pretty simple, just replace
,with\,in a couple more places that need it. Also needed some small logic to make sure we don't double-escape when depickling names that are already escaped.This is risky mostly in multi-targeting scenarios: 4.0 compiler escapes type names differently than 3.X compiler -- can each understand the output of the other?
Added a general-purpose mini test framework for validating such things, which cycles through various cross-version scenarios and makes sure everything plays nicely.