Skip to content
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

MaxChainedCallsOnSameLine reports fully-qualified class names #5028

Closed
dzirbel opened this issue Jul 1, 2022 · 2 comments · Fixed by #5036
Closed

MaxChainedCallsOnSameLine reports fully-qualified class names #5028

dzirbel opened this issue Jul 1, 2022 · 2 comments · Fixed by #5036
Labels

Comments

@dzirbel
Copy link
Contributor

dzirbel commented Jul 1, 2022

Expected Behavior

The MaxChainedCallsOnSameLine should probably ignore fully-qualified class names (like this example from detekt's codebase). These call chains are somewhat different in nature from its intended purpose, and the rule probably shouldn't have a second effect of restricting fully-qualified class names, which may be necessary (or at least preferred over named imports with as) when classes with the same name but different packages are used. See discussion in #5020.

A fix would likely require type resolution to determine the role of chained calls, but this complicates the rule and makes it harder to be applied, just to avoid a fairly rare edge case. So I wanted to open this issue for discussion/feedback on how to proceed.

Observed Behavior

MaxChainedCallsOnSameLine reports fully qualified class names above its maxChainedCalls limit.

Your Environment

  • Version of detekt used: 1.21.0-RC2
@dzirbel dzirbel added the bug label Jul 1, 2022
@BraisGabin
Copy link
Member

I know that rules that doesn't need type solving are better than those others than need it. But I think that it's even more important to don't have false positives. Those positives undermine the confident on detekt so we should fix them, even if they make the rule more complex.

@dzirbel
Copy link
Contributor Author

dzirbel commented Jul 4, 2022

Yeah, I agree this should be fixed even if it requires type resolution. I took a stab at it and couldn't figure out how to extract whether a qualified call was part of a class name from the BindingContext. In particular, when testing with kotlin.collections.ArrayList I couldn't see any indication of how to differentiate that from e.g. foo.bar.ArrayList where in package foo.bar we have fun ArrayList() = kotlin.collections.ArrayList(). If you do foo.bar.ArrayList() that will still be a valid expression with resulting type ArrayList, but this one should be warned for where we want to avoid a warning for kotlin.collections.ArrayList(). Perhaps that's a symptom of my underlying hesitance - a fully-qualified class name isn't really a different kind of syntax as a function call/etc, so perhaps this rule should cover it too.

All that to say: I'd appreciate it if anyone more familiar with type resolution would be able to pick up this issue, since I'm going to give up on it, at least for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants