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

[JS] Improve resolving this in ClassElements and in arrow functions #5995

Merged

Conversation

matthiasblaesing
Copy link
Contributor

Inside initialisation statements:

class Test {
	var1 = 123;
	var2 = this.var1;
}

this needs to evaluate to the surrounding class, to handle this
correctly, search for the right definition of "this" needs to start
at the surrounding object, not the declaration scope of the wrapping
function.

Before:
image

After:
image

In arrow functions this is not redefined, but inherited from the outer
scope. In this sample:

class Foo {
	bar = {};
	
	test(someArray) {
		this.bar = {}; // Mark
		someArray.forEach((val) => {
			this.bar[val] = 1;  // Mark
		});
	}
}

this in the lines with the "Mark" comment is an instance of Foo and
thus it is always the same bar. The this scanning needs to skip arrow
functions.

Before (notice, that only property under the cursor is highlighted):
image

After (notice, that now all occurences are marked):
image

After:

Closes: #5740
Closes: #4376

Inside initialisation statements:

class Test {
	var1 = 123;
	var2 = this.var1;
}

this needs to evaluate to the surrounding class, to handle this
correctly, search for the right definition of "this" needs to start
at the surrounding object, not the declaration scope of the wrapping
function.

In arrow functions this is not redefined, but inherited from the outer
scope. In this sample:

class Foo {
	bar = {};
	
	test(someArray) {
		this.bar = {}; // Mark
		someArray.forEach((val) => {
			this.bar[val] = 1;  // Mark
		});
	}
}

this in the lines with the "Mark" comment is an instance of `Foo` and
thus it is always the same bar. The this scanning needs to skip arrow
functions.

Closes: apache#5740
Closes: apache#4376
@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 May 27, 2023
@matthiasblaesing matthiasblaesing added this to the NB19 milestone May 27, 2023
@matthiasblaesing
Copy link
Contributor Author

@qmegas thank you for testing this.

I plan to merge this next weekend to give people time to review.

@matthiasblaesing
Copy link
Contributor Author

Hearning no objections, will merge this.

@matthiasblaesing matthiasblaesing merged commit 5ebcd00 into apache:master Jun 4, 2023
34 checks passed
@matthiasblaesing matthiasblaesing deleted the javascript_this_scoping branch June 4, 2023 17:18
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
1 participant