-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Don't crash if the root of a trace is a TypeParameter #427
Conversation
@evant this is making debugging pretty difficult in one of my projects. Any chance it can get merged in soon? |
6ea5548
to
e72dd9b
Compare
} else if (type.declaration is KSTypeParameter) { | ||
typeRef.toString() |
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.
This won't completely fix this as a type param can be arbitrarily nested, for example changing your test from
val c: C
to
val c: () -> C
will cause the same exception.
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.
maybe we could wrap toTypeName()
in a try/catch and fallback to typeRef.toString()
for now?
As an aside, I really wish ksp's toString()
properly showed type params, so we wouldn't need to rely on kotlinpoet at all. I submitted a pr to fix this but it's been ignored 😐 google/ksp#1737
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.
I updated the test to reflect that and changed to try catch.
detekt errors, |
Done |
Fixes #426
I'm not sure if this is the best solution, or why the type parameter is included in the CycleDetector. An alternate option is to catch this in the
CycleDetector
and just filter it out. Another option is to catch any exceptions fromtoTypeName()
and usetypeRef.toString()
instead.