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

Parsing issues: Babel, BigInt and Tildot #2503

Closed
Chris-Hibbert opened this issue Feb 22, 2021 · 19 comments · Fixed by #3082
Closed

Parsing issues: Babel, BigInt and Tildot #2503

Chris-Hibbert opened this issue Feb 22, 2021 · 19 comments · Fixed by #3082
Labels
agoric-cli package: agoric-cli documentation Improvements or additions to documentation enhancement New feature or request needs-design question Further information is requested tooling repo-wide infrastructure

Comments

@Chris-Hibbert
Copy link
Contributor

Chris-Hibbert commented Feb 22, 2021

What is the Problem Being Solved?

We're currently stuck on an old version of Babel in order to maintain Tildot support. This is preventing some upgrades, including support for BigInt literals. (We can now do 100n, but 100_000_000n doesn't work in some places.)

This issue is a place to settle on an approach.

Description of the Design

agoric-sdk doesn't currently support underscore separators (1_000_000 is legal javascript). The Babel parser version we're using in a few places doesn't support all the current JS syntax. We can't upgrade without breaking tildot

We have added support for tildot to Babel; it's currently only enabled in the REPL.

  • We haven't been able to turn it on elsewhere because we don't have compatibility support for eslint or the IDEs
  • Our tildot support only works in older versions of Babel, and bringing it up-to-date would be a chore.

Use of E.G() was rhetorically connected to this discussion, but is pretty much separate, since tildot doesn't support destructuring assignments. so E.G() will be discussed in a separate issue.

One coherent proposal is to stop using Babel. This would drop our current support for tildot in the REPL. We could then move to most-recent versions of everything else, and get consistent support for parsing bigints everywhere. Tildot would then only get support by moving babel forward, and getting official support from TC39.

@michaelfig described the process to get tildot adopted:

We need to get the tildot proposal fully specified and support merged to Babel upstream. That will take some design work to satisfy the Babel maintainers that we cover all the corner cases in the syntax.

Security Considerations

All paths lead to parsing javascript in standards compliant ways.

Test Plan

We probably need some new tests with underscores. The main plan is to verify that everything passes CI tests when we add tests that declare values with underscores. I'll also check the REPL parsing, which has its own tests.

@Chris-Hibbert Chris-Hibbert added enhancement New feature or request agoric-cli package: agoric-cli Beta needs-design documentation Improvements or additions to documentation question Further information is requested tooling repo-wide infrastructure labels Feb 22, 2021
@Chris-Hibbert Chris-Hibbert added this to the Beta Initial Launch milestone Feb 22, 2021
@katelynsills
Copy link
Contributor

I don't think tildot is giving us much of anything. It won't be a standard for a long time, it's confusing when we already have E() (we shouldn't have two divergent ways of doing something), and I personally find it harder to read. I say don't worry about breaking tildot support - it's not something that makes sense to support for users right now.

@dckc
Copy link
Member

dckc commented Feb 22, 2021

Coincidentally, I just ran into a piece of code that's crying out for underscores in numerals:

          const WEI_PER_ETHER = BigNumber(1000000000000000000)

-- tx_csv_eth.html line 9

(though 1_000_000_000_000_000_000 is still pretty hard to read. BigNumber(`1${'0'.repeat(18)}`) might be easiest, assuming BigNumber takes strings.)

@FUDCo
Copy link
Contributor

FUDCo commented Feb 22, 2021

(though 1_000_000_000_000_000_000 is still pretty hard to read. BigNumber(`1${'0'.repeat(18)}`) might be easiest, assuming BigNumber takes strings.)

Not sure what BigNumber is, but you can say BigInt(1e18) and it will do the right thing.

@erights
Copy link
Member

erights commented Feb 22, 2021

BigInt(1e18)

That works for 1e18 but not for 1e23. Really. 1e18 is that close to the edge.

OTOH 10n**18n works exactly as well as 10n**23n

@erights
Copy link
Member

erights commented Feb 22, 2021

@dckc yes, BigInt takes strings. BigInt(`1${'0'.repeat(18)}`) does work. I think 10n**18n is clearer.

@erights
Copy link
Member

erights commented Feb 22, 2021

@Chris-Hibbert I fixed the link in your initial comment. In that position the normal github #2502 was being taken as a URL fragment rather than the special github interpretation.

@Chris-Hibbert
Copy link
Contributor Author

No one made any arguments in favor of preserving tildot in the near term, or for continuing to rely on Babel. @warner is out till Monday, so I propose we consider this discussion closed and the decision made to drop Babel unless someone brings up countervailing reasons by EOD 3/1/2021.

@dckc dckc removed their assignment Feb 26, 2021
@warner
Copy link
Member

warner commented Mar 1, 2021

I like tildot for its conciseness in talking about code (I find I use it in comments all the time), but I'll admit I rarely use it in actual executed code, probably because it confuses my editor. (My editor is also really confused about 1n BigInt literals, so I'm not prepared to defend it very much). I should note that I hardly ever write contract code, so I'm not the best person to listen to.

I agree with @katelynsills 's point about the problem of having more than one way to do it. I find tildot easier to read than E() (and I can never remember E().G at all), and there's a slight pedagogical benefit to showing the parallel between x.foo() and x~.foo(), but neither is strong enough for me to recommend keeping it in the face of the other obstacles it raises. I care more about underscores in numeric literals and BigInt literals than tildot. Things will get easier if/when it gets far enough through the standards process to get editor/IDE/toolchain support.

So I guess I'd support a plan to drop tildot for now. I think of @erights is the primary proponent for tildot, and I wouldn't want to abandon something that he cares deeply about, so I'd leave the decision up to him. If we do remove it, I can imagine a future release note that says "new and more pleasant way to invoke eventual-sends" when we bring it back.

We currently support tildot in two ways. Contract code (that goes into a bundle) gets to use tildot because we've added a transformation function into the rollup config that gets used by bundleSource. REPL code gets tildot via vatPowers.transformTildot, which is handed to every root object, which our REPL then passes down to the code that does eval in a new compartment. If we remove tildot support, we could simplify both of these places: I'd probably recommend removing the vatPowers.transformTildot function entirely.

@FUDCo
Copy link
Contributor

FUDCo commented Mar 1, 2021

I fear I must confess I'm not in love with the tildot syntax the way I was with the ! syntax we were forced to abandon. We've told ourselves various comforting stories about the nice parallel with &. and so forth to try to talk ourselves (and others) into investing in it, but visually and typographically I continue to find ~. a bit of a clunker. It would be great if we could come up with a clean direct syntax for eventual send, but until then experience has taught us we can live with E().

@erights
Copy link
Member

erights commented Mar 1, 2021

(and I can never remember E().G at all)

It is E.G() ;)

It is about to be E.get() whose meaning IMO is clearer and more memorable.

dckc added a commit that referenced this issue Mar 9, 2021
* feat(ava-xs): install as npm bin

* chore(avaHandler): SES Compartment already has harden

* feat(ava-xs): provide makeKind, weakStore to test compartment

* feat(ava-xs): handle ava.files, ava.require from package.json

 - support -v / --verbose
 - xsnap: add glob, @agoric/assert dependencies.
   hm... having ava-xs in the xsnap package is increasingly awkward

* chore(avaXS): log error before losing full message

* feat(ava-xs): test.todo

* feat(ava-xs): exclude takes a list of strings

* test(zoe): XS test exclusions with reasons

* test(zoe): add use --verbose on XS to know which script failed

  - add a separate test:xs-unit-debug script,
    since `ava-xs --verbose --debug` isn't yet supported

* test(zoe): postpone _ in underscores until #2503 is done

* test(zoe): be sure async tests wait for all promises

* test(zoe): handle __dirname for bundling zcfTesterContract

 - clean up some lint

* test(zoe): be flexible about error message from assertIssuerKeywords

* test(zoe): support test.failing on XS foro test-zoeHelpersWZcf.js

 - elaborate diagnostics for checkExpectation

* test(zoe): add test:xs-unit, test:xs-worker to test script

* fix(ava-xs): handle missing ava-xs config section

* feat(ava-xs): report count of tests, each in their own crank

* test(ERTP): use ava-xs bin, ava-xs.exclude

* fix(ava-xs): count where we can see crank boundaries

The code running inside xsnap can't see crank boundaries, so don't
bother to count planned assertions there. Count on the node parent
side.

* style(ava-xs): clarify type in avaHandler

* fix(ava-xs): notThrows() success case

* feat(ava-xs): summarize failed tests

* test(zoe): update list of XS exclusions

* test(zoe): postpone test:xs-worker

* test(zoe): postpone test-scriptedOracle

due to intermittent SEGV

* refactor(ava-xs): export powerless main

* test(zoe): try longer timeout

* fix(ava-xs): use cjs rather than -r esm in #!

* test(zoe): postpone zoeHelpers on XS

* style(ava-xs): jsdoc idiom

Co-authored-by: Kris Kowal <[email protected]>

* style(ava-xs): jsdoc idiom

Co-authored-by: Kris Kowal <[email protected]>
@rowgraus rowgraus removed this from the Beta Initial Launch milestone Mar 29, 2021
@erights
Copy link
Member

erights commented Apr 23, 2021

At #2951 we decided to remove tildot support. Use `E.G' when appropriate to get promises for properties without stalling.

@erights
Copy link
Member

erights commented Apr 23, 2021

Or rather, E.get

@warner
Copy link
Member

warner commented Apr 24, 2021

TIldot has been removed. Somebody (@Chris-Hibbert ?), what's the next step for this issue?

@dckc
Copy link
Member

dckc commented Apr 24, 2021

IIRC, the next step is to replace dependencies on @agoric/babel-parser with the current babel parser, which does things like underscores in numeric literals.

I'm struggling to find the source of @agoric/babel-parser... but maybe that doesn't matter.

@michaelfig
Copy link
Member

I'm struggling to find the source of @agoric/babel-parser... but maybe that doesn't matter.

Fwiw, all Agoric forks are kept in agoric-labs.

@dckc
Copy link
Member

dckc commented Apr 24, 2021

I'm struggling to find the source of @agoric/babel-parser... but maybe that doesn't matter.

Fwiw, all Agoric forks are kept in agoric-labs.

I looked in agoric-labs. I didn't find anything with a package.json that says its name is @agoric/babel-parser.

@michaelfig
Copy link
Member

I looked in agoric-labs. I didn't find anything with a package.json that says its name is @agoric/babel-parser.

I was afk, but here I am again: https://github.com/agoric-labs/babel/pulls

@Chris-Hibbert
Copy link
Contributor Author

I'm struggling to find the source of @agoric/babel-parser... but maybe that doesn't matter.

I think it doesn't matter. When I grep for @agoric/babel-parser, I find it in the package.json files for cosmic-swingset, SwingSet, bundle-source, dapp-svelte-wallet, and transform-eventual-send.

Some of those are unused in imports, others only in tests and one comment.

bundle-source/src/index.js and SwingSet/src/controller.js appear to be the two places that need to make an actual change to a more modern parser.

@michaelfig
Copy link
Member

I think it doesn't matter. When I grep for @agoric/babel-parser, I find it in the package.json files for cosmic-swingset, SwingSet, bundle-source, dapp-svelte-wallet, and transform-eventual-send.

bundle-source/src/index.js and SwingSet/src/controller.js appear to be the two places that need to make an actual change to a more modern parser.

Please excise it from everything except for transform-eventual-send (which actually needs it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cli package: agoric-cli documentation Improvements or additions to documentation enhancement New feature or request needs-design question Further information is requested tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.