Use Roslyn to parse type names #45
Conversation
javiercn
force-pushed
the
javiercn/stop-parsing-typenames-manually
branch
from
2cd0e18 to
4edff18
Compare
| { "Foo", true, "", "Foo"}, | ||
| { "SomeProject.Foo", true, "SomeProject", "Foo"}, | ||
| { "SomeProject.Foo<Bar>", true, "SomeProject", "Foo<Bar>"}, | ||
| { "SomeProject.Foo<Bar.Baz>", true, "SomeProject", "Foo<Bar.Baz>"}, | ||
| { "SomeProject.Foo<Bar.Baz>>", true, "", "SomeProject.Foo<Bar.Baz>>"}, | ||
| { "SomeProject.Foo<Bar.Baz>>", true, "SomeProject", "Foo<Bar.Baz>>"}, |
There was a problem hiding this comment.
This is weird. Is it testing for a syntax error? Do we care about what the parsing output would be in that case?
There was a problem hiding this comment.
I don't think we care about these cases. I'm not sure they represent "real" scenarios and the logic for parsing is "dubious" at the very least, so now we at least we don't do our own parsing.
| { | ||
| case GenericNameSyntax genericNameSyntax: | ||
| typeName = genericNameSyntax.ToFullString(); | ||
| typeNamespace = ""; |
There was a problem hiding this comment.
When does this happen? Getting an empty namespace is pretty strange. But looking at the test cases, I didn't spot one where the namespace actually came back as empty (at least, when type was fully qualified).
There was a problem hiding this comment.
I might add some additional test cases, I didn't want to invest more time on it before validating the approach E2E with the tests. I also need to check with tooling folks if there's any drawback WRT to tooling scenarios (which I don't think there is)
There was a problem hiding this comment.
I edited my comment before I saw your reply, but possibly after you posted your reply.
at least, when type was fully qualified
OK, so I guess it's more that "namespace is only given when the syntax is QualifiedNameSyntax" in which case cool.
|
There's a follow-up PR to get rid of more "in-house" type parsing in favor of leveraging roslyn APIs following the same approach employed here. |
|
@pranavkm that was one of my concerns too, maybe we can talk about it more in depth. I was focusing on correctness first and avoiding our whacky parsing, but we can probably make it more efficient. |
|
@javiercn can we merge this? |
|
@pranavkm can we hold on until we get to the root cause of what's happening on the original issue? @TanayParikh and me looked at this yesterday, and while we think this change is an improvement, we think that the root cause of the issue is that somehow we end up with an invalid |
|
Closing since ca1728b supersedes this. |
Contributes to dotnet/aspnetcore#38879
The exception source seems to be coming from a place where we split a full type name into its type name and namespace. Instead of using our own "dubious" parsing logic, rely on the roslyn APIs to do the parsing.
This entailed adding a new method to
TypeNameFeatureand flowing that to the type that performs this operation.