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.

Sign up for GitHub

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

Jump to bottom

false match on lines ending with import #43

Closed
warner opened this issue Aug 21, 2019 · 2 comments
Closed

false match on lines ending with import #43

warner opened this issue Aug 21, 2019 · 2 comments
Assignees
@erights
Labels
SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Aug 21, 2019

The SES code that prohibits dynamic import expressions doesn't use a full parser, so it gets false matches when the word import appears just before a comment opening token, possibly separated by whitespace. Twice I've had this trigger on comments in my code that look like:

stuff()
// this is an import
// of something
morestuff()

The scanner doesn't know that import is already in a comment, so it is guarding against:

stuff();
import
// sneaky comment
('dynamic module to import');

SES should eventually learn to ignore the first, but for now it would be nice if SwingSet just had a nicer error message. The one you get happens after rollup finishes combining all the files into a single string, and then you get an error referring to the line number in the merged file:

(node:21506) UnhandledPromiseRejectionWarning: SyntaxError: possible import expression rejected around line 2475
    at rejectImportExpressions (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1050:11)
    at rejectDangerousSources (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1087:3)
    at Object.rewrite (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1094:5)
    at allTransforms.reduce.src.src (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1205:61)
    at Array.reduce (<anonymous>)
    at eval (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1204:45)
    at /home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1271:46
    at realmEvaluate (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:1504:10)
    at callAndWrapError (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:82:14)
    at Realm.evaluate (/home/warner/bindmounts/trees/SwingSet/node_modules/ses/dist/ses.cjs.js:162:14)
(node:21506) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:21506) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I'm thinking maybe we scan the rollup output for /\bimport\s*$/ and then if eval() throws, we print the full line that matched, rather than just the line number of a string that only ever existed in memory. Not sure quite where we'd do this, it may have to go into SES itself.
@warner warner transferred this issue from Agoric/SwingSet Dec 1, 2019
@warner warner added the SwingSet package: SwingSet label Dec 1, 2019
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
@sunnya97
Merge pull request Agoric#43 from cosmos/sunny/combine-stores
527b9fd 
R4R: Combined three stores into one with Whois struct as value  Agoric#2
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
Three stores had been combined in Agoric#43.
027c133
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
@jackzampolin
Merge PR Agoric#58: Small docs fix
4062955 
Three stores had been combined in Agoric#43.
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
@warner
prevent vats from using send() to call themselves
274e6bc 
Vats normally use send() to queue messages for things they've
imported (either the exports of other vats, or kernel promises which
hopefully resolve to an export of some other vat).

But if a Vat uses send() on one if its own exports, that will queue a message
which will be delivered back to themselves later.

There's currently no good reason to do this, and it probably indicates the
vat got confused about what it was passing to send(). So for now, detect this
and throw an exception.

This might change when we add escalators later, but even then I can't
currently imagine a scenario in which we want to defer execution that isn't
more efficient to express with a local `Promise.resolve`.

closes Agoric#43
dckc pushed a commit to dckc/agoric-sdk that referenced this issue Dec 5, 2019
@michaelfig
Merge pull request Agoric#43 from Agoric/insecure-evaluate
e8726e1 
@agoric/evaluate: use published version 1.1.0
@warner warner mentioned this issue Feb 7, 2020
Closed
@erights erights self-assigned this Feb 26, 2021
@erights
Copy link
Member

erights commented Feb 26, 2021

@warner I'm assigning to myself and closing, under the assumption that this is fixed enough and is not currently an active thorn in our side. If I need to do more on this, please reopen and leave assigned to me.

@erights erights closed this as completed Feb 26, 2021
@softmarshmallow
Copy link

Not so related, but i'm having this Error: SyntaxError: possible import expression rejected around line error also, cannot find any other reference related to this error message but here. can you share us some context and what caused and how solved the problem?

This was referenced Jun 6, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erights erights

Labels
SwingSet package: SwingSet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@warner @erights @softmarshmallow