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

The toString() of 'class' and 'function' return SourceText #1620

Closed
wants to merge 0 commits into from

Conversation

scgm0
Copy link
Contributor

@scgm0 scgm0 commented Aug 16, 2023

Probably not the best, I just don't know how to describe it in the issue(

Edit:
I noticed that the previous reason for returning native code was that Esprima didn't support it (in the ToString comment for FunctionInstance), but I found that Esprima now supports returning SourceText, so wanted to align with the behaviour of the mainstream js engines.

@scgm0 scgm0 changed the title Fix some minor issues with ScriptFunctionInstance Fix some minor issues Aug 18, 2023
@scgm0 scgm0 changed the title Fix some minor issues The toString() of 'class' and 'function' return SourceText Aug 21, 2023
@scgm0
Copy link
Contributor Author

scgm0 commented Aug 23, 2023

@lahma What do you think? I'm not familiar with c# and jint....

@lahma
Copy link
Collaborator

lahma commented Aug 23, 2023

Well I think that this PR is missing a description about what is trying to be achieved and why. Also there's no test case(s) for changes that would show how things are improved. Tests are also failing.

@scgm0
Copy link
Contributor Author

scgm0 commented Aug 23, 2023

Well I think that this PR is missing a description about what is trying to be achieved and why. Also there's no test case(s) for changes that would show how things are improved. Tests are also failing.

Implement the target:
"class" and "function" toString() returns SourceText
I noticed that the previous reason for returning native code was that Esprima didn't support it (in the ToString comment for FunctionInstance), but I found that Esprima now supports returning SourceText, so wanted to align with the behaviour of the mainstream js engines.

@scgm0
Copy link
Contributor Author

scgm0 commented Aug 23, 2023

Well I think that this PR is missing a description about what is trying to be achieved and why. Also there's no test case(s) for changes that would show how things are improved. Tests are also failing.

The Tests should be fine now, on my computer only some of the Date tests failed, this should just be a problem with my computer settings.

@scgm0
Copy link
Contributor Author

scgm0 commented Aug 23, 2023

@lahma Found out that the SourceText returned by Esprima is currently missing Comments, which isn't perfect, but I think it's still a bit better than just returning native code.
Esprima seems to be mainly used in Jint, maybe Jint can ask for relevant updates?
(My English is terrible, I hope Deepl's translation is correct)

@lahma
Copy link
Collaborator

lahma commented Aug 23, 2023

There are many failing tests: https://github.com/sebastienros/jint/actions/runs/5948466549?pr=1620 , also no new test case to show what was fixed.

@scgm0
Copy link
Contributor Author

scgm0 commented Aug 23, 2023

There are many failing tests: https://github.com/sebastienros/jint/actions/runs/5948466549?pr=1620 , also no new test case to show what was fixed.

The tests that failed were all Function/prototype/toString related in Test262, because the SourceText returned by Esprima currently does not contain Comments, and the tests in Test262 require the SourceText returned by toString to contain Comments.
If Esprima does not update this feature, this part of the test will have to be skipped.

@scgm0
Copy link
Contributor Author

scgm0 commented Dec 28, 2023

@lahma Trying to pass the test requires Esprima to update the relevant content, is it possible to merge pr first and wait for Esprima to update?

@lahma
Copy link
Collaborator

lahma commented Dec 28, 2023

Are you working on updating esprima somehow?

@scgm0
Copy link
Contributor Author

scgm0 commented Dec 28, 2023

Are you working on updating esprima somehow?

I'm sorry to say that I tried, but it's beyond my current capabilities(

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