Skip to content

Emit consistent position meta on fn capture traces #12033

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

Merged
merged 6 commits into from
Aug 2, 2022

Conversation

lukaszsamson
Copy link
Contributor

While this PR fixes the bug, I'm not really sure about my approach.
Is merging the capture and dot meta keywords acceptable? Or should I explicitly pass the dot meta to trace emit?
Do we want capture_meta emitted on tracer?
Is it OK that :no_parens meta gets passed when expanding macros (see comment in expansion_test.exs)

Fixes #12023

@josevalim
Copy link
Member

Thank you!

Merging is not acceptable. We don't perform this anywhere else and this may lead to subtle behaviour (for example, propagating the hygiene information of / to other places).

I think you should just pass the RequireMeta and ImportMeta as the AST, without merging. The tests show this is a good change IMO.

Do we want capture_meta emitted on tracer?

It is up to you. If you need this information, then capture_meta is the way to go, but we should submit it only in the tracer.

@lukaszsamson
Copy link
Contributor Author

lukaszsamson commented Aug 2, 2022

I think you should just pass the RequireMeta and ImportMeta as the AST, without merging. The tests show this is a good change IMO.

I originally started this PR from 1.13 branch and there doing that crashed import capture expansion with

== Compilation error in file test/elixir/kernel/quote_test.exs ==
** (CompileError) test/elixir/kernel/quote_test.exs:585: undefined function length/1 (expected Kernel.QuoteTest.ImportsHygieneTest to define such a function or for it to be imported, but none are available)

on 1.14 and master this works.

It is up to you. If you need this information, then capture_meta is the way to go, but we should submit it only in the tracer.

I don't need that currently. In references provider I only trace when a symbol (fun in that case) is used and I'm not interested in how it's used.

@josevalim josevalim merged commit 7d8d43b into elixir-lang:main Aug 2, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@lukaszsamson lukaszsamson deleted the ls-capture-pos-4 branch April 26, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent code positions reported by tracer
2 participants