-
Notifications
You must be signed in to change notification settings - Fork 57
[generator] Fix invalid parsing of complex generic types. #729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Assert.AreEqual ("global::System.Collections.Generic.IDictionary<global::System.Collections.Generic.IList<string>, global::Kotlin.Pair>", | ||
opt.GetOutputName ("System.Collections.Generic.IDictionary<System.Collections.Generic.IList<string>, Kotlin.Pair>")); | ||
|
||
Assert.AreEqual ("global::System.Collections.Generic.IDictionary<global::System.Collections.Generic.IList<string>, global::System.Collections.Generic.IList<global::Kotlin.Pair>>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all "positive" tests.
You should also test "negative" tests, "invalid" types. What happens if it attempts to parse System.Collections.Generic.List<>
(missing type) or System.Collections.Generic.List<<>
(too many <
s) or System.Collections.Generic.List<>>
(too many >
s)?
Presumably it fails; does it? Does it fail in the way we expect? (Or does it instead return an empty string?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also try nested generic types, a'la
List<List<string>.Enumerator[]>
.
Assert.AreEqual ("void", opt.GetOutputName ("void")); | ||
Assert.AreEqual ("void", opt.GetOutputName ("System.Void")); | ||
Assert.AreEqual ("params int[]", opt.GetOutputName ("params int[]")); | ||
Assert.AreEqual ("params global::System.Object[]", opt.GetOutputName ("params System.Object[]")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also check arrays-of-arrays, e.g. int[][][]
.
opt.GetOutputName ("System.Collections.Generic.Dictionary<string, string>")); | ||
|
||
Assert.AreEqual ("global::System.Collections.Generic.List<global::System.Collections.Generic.List<string>>", | ||
opt.GetOutputName ("System.Collections.Generic.List<System.Collections.Generic.List<string>>")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also try intermixing arrays & generics, e.g. System.Collections.Generic.List<System.Collections.Generic.List<string>[]>[]
.
2d8bfe9
to
2ba8bd7
Compare
Thanks for the additional test cases, they revealed a few more issues. Granted, they didn't work previously so it wouldn't have been a regression, but now we handle those cases as well! The behavior with invalid types is "undefined". It may return an invalid type string or it may throw an Luckily at this point we are simply outputting types that have already been validated by other aspects of |
Fixes: #728
In order to prevent namespace clashes we always write non-intrinsic types prefixed with
global::
.For example:
becomes
To do this, we have to parse a type into its component types so we can add
global::
to each part. However our parsing algorithm struggles with generic types with multiple type arguments.Thus:
becomes
and
throws an
ArgumentOutOfRangeException
.This PR introduces a new recursive parser that better understands generic types and generates the expected outputs.