Skip to content

JS: Improve handling of const keyword#8434

Merged
ebarboni merged 1 commit intoapache:deliveryfrom
matthiasblaesing:block_scope
Apr 23, 2025
Merged

JS: Improve handling of const keyword#8434
ebarboni merged 1 commit intoapache:deliveryfrom
matthiasblaesing:block_scope

Conversation

@matthiasblaesing
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing commented Apr 20, 2025

  • const declared variables must be treated as block scoped declarations, as let declared variables are already
  • Highlighting of const is aligned with let

Closes: #8299

- `const` declared variables must be treated as block scoped
  declarations, as `let` declared variables are already
- Highlighting of `const` is aligned with `let`

Closes: apache#8299
@matthiasblaesing matthiasblaesing added JavaScript [ci] enable web job and extra JavaScript tests (webcommon/javascript2.editor) ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Apr 20, 2025
@Override
public boolean enterVarNode(VarNode varNode) {
if (varNode.isLet()) {
if (varNode.isLet() || varNode.isConst()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not use varNode.isBlockScoped() ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the usage of let or const implies block scope, while the reverse might not be true (in a hypothetical JS extension) in the future. It is true that at this point in time varNode.isLet() || varNode.isConst() and varNode.isBlockScoped() result in identical behavior, but it is not guaranteed to stay that way.

In the concrete case, the analyser explicitly handles the two keywords let and const in let or const declarations nothing more.

@NReib
Copy link
Copy Markdown
Contributor

NReib commented Apr 21, 2025

This fixes the issue for me

@matthiasblaesing matthiasblaesing changed the base branch from master to delivery April 21, 2025 19:26
@apache apache locked and limited conversation to collaborators Apr 21, 2025
@apache apache unlocked this conversation Apr 21, 2025
@matthiasblaesing
Copy link
Copy Markdown
Contributor Author

@NReib thanks for testing/checking

@matthiasblaesing matthiasblaesing added this to the NB26 milestone Apr 21, 2025
@ebarboni
Copy link
Copy Markdown
Contributor

merging for 26-rc2, checking were not ok yesterday but it's green today :D

@ebarboni ebarboni merged commit ed9803b into apache:delivery Apr 23, 2025
62 checks passed
@matthiasblaesing
Copy link
Copy Markdown
Contributor Author

@ebarboni yeah, @neilcsmith-net restarted the testrun before I could look into it. Thanks

@neilcsmith-net
Copy link
Copy Markdown
Member

@matthiasblaesing logs are still at https://github.com/apache/netbeans/actions/runs/14579882647 (attempts top right).

They failed on different tests, or I wouldn't have restarted.

@matthiasblaesing
Copy link
Copy Markdown
Contributor Author

@neilcsmith-net I'm aware of that. Just FYI we also had situations where the same test failed multiple times.

@neilcsmith-net
Copy link
Copy Markdown
Member

Yes, in which case the process would have been different. I checked the failed tests were different, I checked there wasn't anything obvious in your changes that would have triggered the failure, then I restarted again. That's how we've always handled. I'm not sure what your concern is or why it's stopping you looking into anything?

@matthiasblaesing
Copy link
Copy Markdown
Contributor Author

Nothing is wrong, I just reacted to the implication, that I would not be aware of how github handles test restarts.

@matthiasblaesing matthiasblaesing deleted the block_scope branch October 5, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) JavaScript [ci] enable web job and extra JavaScript tests (webcommon/javascript2.editor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[JS] Constant highlighting with inner function declaration and default arguments

4 participants