mbuscemi / elm-lens

Elm code visualizations for maximum productivity in Atom
https://atom.io/packages/elm-lens
MIT License
51 stars 2 forks source link

Wrong reference count (Task) #8

Closed Couto closed 6 years ago

Couto commented 6 years ago

This happens consistently in at least two projects.

I'm not an expert in Elm, but I believe that is probably because is doing some confusion with Platform.Task

screen shot 2017-12-29 at 12 55 54

You can see the source code in action in this ellie: https://ellie-app.com/cvmGqQdtda1/0

If I rename Task to TaskItem per example, the reference count becomes correct:

screen shot 2017-12-29 at 13 05 44

mbuscemi commented 6 years ago

Thanks for the report! I'm looking into this and I'll follow up when I have a concrete plan.

mbuscemi commented 6 years ago

The Problem: In order to count references, the current implementation has lists of references that it walks and counts up the matches. When I noticed the algorithm collecting up tons of references to String, Int, etc., I implemented a blacklist on many auto-imported types and functions, figuring they didn't need to be counted: https://github.com/mbuscemi/elm-lens/blob/master/elm/src/ElmFile/References.elm#L151.

It looks like I didn't appropriately anticipate actual usage.

Solution: I implemented the list approach to reference counting because it was quick, but I'm going to move to a dictionary implementation, which will obviate the blacklist. That should fix this issue. I'll make this work part of 0.2.0.

Thanks again, @Couto!

Couto commented 6 years ago

Thanks a lot for the update explaining the problem! That's pretty cool! 👍

stil4m commented 6 years ago

Encountered the same issue when using this on the elm-analyse code base. Great, that is already reported +1.

mbuscemi commented 6 years ago

This should be fixed in the 0.2.0 branch.

mbuscemi commented 6 years ago

@Couto @stil4m Can you please confirm that this is fixed for you in v0.2.0?

stil4m commented 6 years ago

Tested in 0.2.0 and it is fixed 👍

Couto commented 6 years ago

@mbuscemi I can also confirm that this is fixed (at least for my use cases)! 👍

Thanks for your work! 🥇

mbuscemi commented 6 years ago

Verified fixed.