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

lexical-binding breaks decl-scan #25

Open
niklas opened this issue Oct 28, 2024 · 1 comment · May be fixed by #27
Open

lexical-binding breaks decl-scan #25

niklas opened this issue Oct 28, 2024 · 1 comment · May be fixed by #27

Comments

@niklas
Copy link

niklas commented Oct 28, 2024

Hemlo!

Since 9a9f550 the function purescript-ds-create-imenu-index throws an error:

Scanning declarations in Helpers.purs... (  0%)
In ‘Imenu’ source: ‘helm-imenu-candidates’ 
 (void-variable index-imp-alist)

With some creative insertion of (message ..) I managed to find the guilty expression: (symbol-value sym) in line 497 (https://github.com/purescript-emacs/purescript-mode/blob/master/purescript-decl-scan.el#L497).

I am still quite a elisp-noob, so I cannot explain why the index-imp-alist variable is void, because I see it initialized in the (let* ..) in line 463ff.

When I remove the lexical-binding: t in the first line of the file, the error disappears and my helm/imenu declaration list is properly populated.

Emacs Versions tested: 26.X, 29.4, 31.0

Hi-Angel added a commit to Hi-Angel/purescript-mode that referenced this issue Nov 5, 2024
The function has implemented a poor man's hash table by manually
enlisting keys and then jumping through the hoops by fetching them
from one place, converting values to symbols to values… In particular,
it was using `symbol-value` to map a symbol to its value, however the
symbol was a local variable, and `symbol-value` doesn't work with them
under lexical-binding, which the file was converted to since commit
9a9f550.

So fix the regression and simplify the code at the same time.

Fixes: purescript-emacs#25
@Hi-Angel
Copy link
Contributor

Hi-Angel commented Nov 6, 2024

Thank you for the report! Please, in the future when you notice someone introduced a regression (disregarding the project), please CC the author, so they'd be able to get involved.

I fixed the bug in a PR, it's waiting for review. I'm not sure how long it's gonna take though because @kritzcreek seems to have disappeared, and from what I understand there isn't other maintainers… In the meanwhile, you can apply the changes locally if you want by copying from the raw version.

I tested the output of the function before and after the change on one PS file and the hashsums match, the output didn't change. So presumably everything should be okay.

Hi-Angel added a commit to Hi-Angel/purescript-mode that referenced this issue Nov 11, 2024
The function has implemented a poor man's hash table by manually
enlisting keys and then jumping through the hoops by fetching them
from one place, converting values to symbols to values… In particular,
it was using `symbol-value` to map a symbol to its value, however the
symbol was a local variable, and `symbol-value` doesn't work with them
under lexical-binding, which the file was converted to since commit
9a9f550.

So fix the regression and simplify the code at the same time.

Fixes: purescript-emacs#25
Hi-Angel added a commit to Hi-Angel/purescript-mode that referenced this issue Nov 11, 2024
The function has implemented a poor man's hash table by manually
enlisting keys and then jumping through the hoops by fetching them
from one place, converting values to symbols to values… In particular,
it was using `symbol-value` to map a symbol to its value, however the
symbol was a local variable, and `symbol-value` doesn't work with them
under lexical-binding, which the file was converted to since commit
9a9f550.

So fix the regression and simplify the code at the same time.

Fixes: purescript-emacs#25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants