Quickfix for #9 and #10

[forki]

The FSI has an issue with , in identifiers.

reported in #9:

// error FS0192: internal error: binding null type in envBindTypeRef
let ``1,`` x = [||] |> Array.fold (+) ",";;

reported in #10:

module ``Generic IKEv1, AuthIP, and IKEv2`` =
  [<Literal>]
  let Category = "Generic IKEv1, AuthIP, and IKEv2";;

Both issue are caused by an encapsulated identifier in modB (see https://github.com/forki/visualfsharp/compare/Microsoft:fsharp4...forki:fsicomma?expand=1#diff-ee96c09c03afcf797a979eb48d7f7f09L1725).

We ask for ***.1,@*** but the dictionary in modB has only a value for ***.1\,@***.

My fix encapsulates the , and this fixes both issues.

I know this is not the correct fix, but it's a start and I'd like to get this in.

1. Since this can only be reproduced in fsi - how can we write a test?
2. Where and how should we fix this? Inside of GetTypeAndLog? Are there other chars that need to be encapsulated?

[dsyme]

Let's get to the bottom of the root causes for the problem. To start with, what happens when "," occurs in a type name?

For example, as I understand it the names in an fsc.exe generated on-disk .DLL do not get escaped. However it seems from the above that "," gets escaped in System.Reflection.* qualified names..

However, the code for ILTypeRef.QualifiedName isn't escaping "," at all. Perhaps it should be. But ILTypeRef.QualfiiedName and friends are used qiute a bit in the codebase, so we have to be careful here.

I'm also aware that "," in type names is used in F# as a special indicator in mangled names for statically parameterized types, see PrettyNaming.demangleProvidedTypeName/mangleProvidedTypeName. These encodings/decodings are set in stone now since they are part of binary compatibility for F#. Given this, it could be that the use of "," in type names should just be disallowed, and that the best fix is just to add "," to IllegalCharactersInTypeAndNamespaceNames. This would be a breaking change, though given that there are bugs in this area already I'd be OK with that.

(BTW one consistent problem in the F# compiler codebase is that we didn't use the F# type system enough to track invariants of strings. For exampe, there's no real reason that QualifiedName and BasicQualfiedName couldn't return a type "QualifiedName" that tracks that the string has been normalized/escaped according to particular rules. We could have avoided bugs if we'd used the F# type system a bit more.)

[forki]

mhm, I'm not a big fan of making the , illegal.
Since it compiles in fsc.exe it's already used in real-world code and probably used in many names for unittests.

/cc @haf

[forki]

BTW: Adding , to IllegalCharactersInTypeAndNamespaceNames in PrettyNaming.fs doesn't work for #9. I assume this is only for identifiers which are not quoted in backticks - or simply it's only used for types and we have a function.

[forki]

I updated the PR. Instead of 475ba2b where I hacked the encapsulation we now disallow , in module and type names. This will "fix" #10 by disallowing the module name.

For #9 I added the replacement and we still use , in function names:

[latkin]

@dsyme does this look like an appropriately general fix at this point?

[dsyme]

Perhaps - I need to look at it more though - am planning to do so. Leave it open for now.

[forki]

#250 looks like a much better fix.