slackhq / compose-lints

Lint checks to aid with a healthy adoption of Compose
https://slackhq.github.io/compose-lints
Apache License 2.0
390 stars 23 forks source link

ComposeModifierReused false positive on a shadowed parameter name #268

Open jzbrooks opened 8 months ago

jzbrooks commented 8 months ago

I was tinkering with some ideas a couple days ago and noticed that this check fails the use of a locally scoped modifier instance if it has the same name as the composable parameter's modifier argument. I would expect the check to take scope into consideration and not fail when the modifier parameter name has been shadowed.

Notice below that the name modifier inside the Row (I wish I'd screencap'd the line numbers). Renaming that variable works around the problem.

Error

Screenshot 2024-02-02 at 10 12 50 AM

Rename Workaround

Screenshot 2024-02-02 at 10 13 27 AM

ZacSweers commented 8 months ago

I think your workaround is probably the right thing to do in this scenario if I'm honest. I'm not sure how easy it is to trace through shadowed names, and this is a compiler warning anyway :)

jzbrooks commented 8 months ago

I'm assuming the local has a different UAST type than the parameter name or that maybe somehow PSI resolution might help do that tracing. I agree it isn't a huge deal, but it seems like a worthwhile endeavor to investigate if there's an easy solution.

I will try to do that soon.

ZacSweers commented 8 months ago

Yeah def open to a PR if you find a good way

jzbrooks commented 8 months ago

I wonder if the path forward involves leveraging DataFlowAnalyzer to track uses of modifiers.

It's not much, but this detects some of the offending errors: https://github.com/slackhq/compose-lints/compare/main...jzbrooks:compose-lints:name-shadowing

If some of the test stubs were more robust (for example Modifier.then doesn't exist in the stubs so the analyzer doesn't know what to do with it). I wanted to spend more time with this over the past week, but I wasn't able to.