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

More precise lookup for string property of AstNameTy #157

Merged
merged 4 commits into from
Jul 21, 2023
Merged

Conversation

doehyunbaek
Copy link
Collaborator

Previously, lookup of string property in AstNameTy did not work and just returned AstTopTy. For example, in es2022, there is a lookup of

let body = script.ScriptBody

in PerformEval

image

And according to the spec, this should evaluate to Ast[StatementList] but did not, causing imprecision in the analysis.

There are six cases in es2022 when such lookup happens.

(Ast[CallMemberExpression],String["MemberExpression"])
(Ast[CallMemberExpression],String["Arguments"])
(Ast[CaseClause],String["Expression"])
(Ast[AsyncArrowHead],String["ArrowFormalParameters"])
(Ast[CallExpression, MemberExpression, OptionalChain],String["parent"])
(Ast[Script],String["ScriptBody"])

In all these cases except the fifth case, which is a lookup of "parent", I confirmed the type being more precise.

This commit fixes the issue.

@doehyunbaek doehyunbaek requested a review from jhnaldo July 17, 2023 05:10
@doehyunbaek doehyunbaek changed the title Astnamety lookup Support lookup of string prop in AstNameTy Jul 17, 2023
@doehyunbaek doehyunbaek changed the base branch from main to dev July 17, 2023 05:11
@doehyunbaek
Copy link
Collaborator Author

doehyunbaek commented Jul 17, 2023

Currently, with es2022, the following ReturnTypeMismatch happens with PerformEval

[ReturnTypeMismatch] PerformEval (step 32, 57:14-32)
- expected: Normal[ESValue] | Abrupt
- actual  : Normal[ESValue | PrivateName | ReferenceRecord | Const[~unused~]]

Why is that? In in step 28.a, the rhs in the below statement, the result of evaluating body contains other types like PrivateName.
image
When we check the type of body in the mentioned nodepoint, we can confirm it is general body: Ast. It is correct that evaluation of general parse node may contain other values like PrivateName, but the imprecise part in this analysis is that:

body is not any Ast, we can be sure body is ScriptBody, whose evaluation can only be of type Normal[ESValue | Const[~empty~]] | Abrupt. Why is this so?

This is because body is the result of ScriptBody of script, as the below step 11.d of PerformEval shows
image

So body is not just any AST but specifically ScriptBody. But this is not handled properly in the current version of ESMeta.

As I debugged the cause, the reason was because lookupAst was not implemented when the ast parameter was of type AstNameTy. In the PerformEval case, the type of script was of AstNameTy, as the below IR is executed by the parse function.

  let script = (parse %3 (nt |Script|[]))

So I copied the lookup logic for AstSingleTy and handled the AstNameTy case analogously. As a result, body is properly constrained to Ast[ScriptBody], result is properly constrained to Normal[ESValue | Const[~empty~]] | Abrupt(the handling of the Const[empty] case is done in the step 29, as below, so there is no more problem), and there is no more ReturnTypeMismatch.
image

Copy link
Contributor

@jhnaldo jhnaldo left a comment

Choose a reason for hiding this comment

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

Please define and use a helper function for the math case.

previously, lookup of string property in AstNameTy did not work and just
returned AstTopTy. For example, in es2022, there is a lookup of

  let body = script.ScriptBody

And according to the [spec](https://tc39.es/ecma262/#prod-ScriptBody),
this should evaluate to Ast[StatementList] but did not, causing imprecision
in the analysis.

This commit fixes the issue.
Copy link
Contributor

@jhnaldo jhnaldo left a comment

Choose a reason for hiding this comment

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

I slightly refactored your code before accepting the PR. Thanks!

@jhnaldo jhnaldo added enhancement Enhance the quality of a feature area:analyzer Related to analyzer area:type Related to types labels Jul 21, 2023
@jhnaldo jhnaldo changed the title Support lookup of string prop in AstNameTy More precise lookup for string property of AstNameTy Jul 21, 2023
@jhnaldo jhnaldo merged commit 7fdf49a into dev Jul 21, 2023
6 checks passed
@jhnaldo jhnaldo deleted the astnamety-lookup branch July 21, 2023 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:analyzer Related to analyzer area:type Related to types enhancement Enhance the quality of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants