Skip to content

fix eval invocation in typescript--re-search-backward to work with lexical scoping #152

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

Conversation

Rogach
Copy link
Contributor

@Rogach Rogach commented Sep 20, 2021

Under lexical scoping variable references won't be found when evaluating the expression, resulting in "eval: Symbol’s value as variable is void: regexp" error.

By the way, do we really need the eval here?

Copy link
Member

@josteink josteink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the need for (eval) myself, but apart from that the change itself looks good to me!

@josteink
Copy link
Member

While the change itself looks good, it reflects on something which used to be broken while the build was still “green”.

Would it be possible to add a test-case for this to prevent future regressions?

@Rogach
Copy link
Contributor Author

Rogach commented Sep 20, 2021

Yes, I'll add the test tomorrow (should be easy - just explicitly set lexical-binding to non-nil).

@Rogach
Copy link
Contributor Author

Rogach commented Sep 20, 2021

Hm, no, seems that it's not that easy - you can't change lexical-binding value during execution, it's only changed on per-file basis or globally.

I asked over in emacs irc and one suggestion was "write a temp file with the setting and code and eval it", but that seems way to involved for a simple test.

One alternative is to switch all tests&code to lexical scoping (seems that lexical scoping is the main way forward nowdays).

@josteink
Copy link
Member

josteink commented Sep 20, 2021

You could always create a dedicated file for tests in lexical mode. We already have different test files for different specific sub-genres of tests, so this would really not be any different.

If you want to switch all the tests over, feel free! It’s by no means a requirement though.

@josteink
Copy link
Member

Also I have fixed the CI so that all the builds are green now, so if you merge in latest changes from master, this PR should be all green as well 🙂

…xical scoping

Under lexical scoping variable references won't be found when evaluating
the expression, resulting in "eval: Symbol’s value as variable is void: regexp"
error.
@Rogach Rogach force-pushed the pr/fix-indentation-when-lexical-scoping branch from 5a8ef32 to bdd6fd4 Compare September 21, 2021 18:25
@Rogach
Copy link
Contributor Author

Rogach commented Sep 21, 2021

I've added the test, but it's horrible (I had to re-read and re-evaluate typescript-mode.el to get it to load with lexical-binding enabled). I'm not even sure how I managed to stumble upon the original bug - it seems that lexical-binding can be enabled only from inside the library file. It's possible that it was some regression inside Emacs itself - I'm running builds from git master, and I can't reproduce my original bug under the latest build.

Also I have fixed the CI so that all the builds are green now, so if you merge in latest changes from master, this PR should be all green as well

I rebased the changes on top of latest master, hopefully the CI runs fine =)

@Rogach Rogach force-pushed the pr/fix-indentation-when-lexical-scoping branch from bdd6fd4 to 47b7fd2 Compare September 21, 2021 18:29
@josteink
Copy link
Member

Lol. That’s absolutely terrible indeed, but it does the job. It also doubles as a preparation for migrating to lexical mode proper.

Consider it merged! Thanks for the patch!

@josteink josteink merged commit c9b22f5 into emacs-typescript:master Sep 21, 2021
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 this pull request may close these issues.

2 participants