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

Bump ecmarkup and enable "user code" annotation #2125

Merged
merged 3 commits into from
Apr 5, 2022
Merged

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Apr 5, 2022

cc @gibson042

See tc39/ecma262#2548 for a description of the annotation. The relevant information is now propagated in the biblio, so it can be used in proposals. In ecmarkup the annotation off by default because it may require manual annotation to be correct, but I've enabled it in this PR: I don't think any sites in this PR need annotation - there's no use of [[Get]] and friends directly, no weird stuff with generators, etc. (I didn't review extremely carefully, though.)

Also

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #2125 (bd9529e) into main (1f3fba8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2125   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files          19       19           
  Lines       10928    10928           
  Branches     1571     1571           
=======================================
  Hits        10075    10075           
  Misses        835      835           
  Partials       18       18           
Flag Coverage Δ
test262 82.60% <ø> (ø)
tests 84.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f3fba8...bd9529e. Read the comment docs.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 5, 2022

I think the codecov bot is broken.

@ptomato
Copy link
Collaborator

ptomato commented Apr 5, 2022

I think the codecov bot is broken.

No, just really slow to execute test262 tests 😛

Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks!

This might be obvious if I look at the documentation for the user code feature in ecmarkup, but should we be specifically annotating when a call site marked "UC" cannot call into user code? e.g. CreateTemporalDate does not call into user code when newTarget is absent, and there are places in the spec text where it is called without a newTarget.

@ptomato ptomato merged commit 34d44a4 into tc39:main Apr 5, 2022
@bakkot bakkot deleted the bump-emu branch April 5, 2022 22:50
@bakkot
Copy link
Contributor Author

bakkot commented Apr 5, 2022

This might be obvious if I look at the documentation for the user code feature in ecmarkup, but should we be specifically annotating when a call site marked "UC" cannot call into user code? e.g. CreateTemporalDate does not call into user code when newTarget is absent, and there are places in the spec text where it is called without a newTarget.

The heuristic ecmarkup uses is that if you are calling something which normally triggers user code, but are using ! to assert that it can't throw in this particular use, that probably means it's not calling user code (otherwise it would have been able to throw). This heuristic is almost perfectly accurate on the main 262 spec, but I don't how how well it works here. At a skim, it looks roughly right for CreateTemporalDate except that there are a bunch of calls to CreateTemporalDate which are ?-prefixed but also don't pass newTarget, which seems wrong to me.

If you are calling something which can sometimes invoke user code in such a way that it cannot invoke user code in this particular usage, but can still throw, you will need to manually suppress the effect, yes.

@ptomato ptomato added the no-spec-text PR can be ignored by implementors label May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants