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

${myFunction} idiom not supported by xs #426

Closed
dckc opened this issue Jan 19, 2020 · 22 comments
Closed

${myFunction} idiom not supported by xs #426

dckc opened this issue Jan 19, 2020 · 22 comments
Assignees
Labels
spawner Spawner and contractHost xsnap the XS execution tool

Comments

@dckc
Copy link
Member

dckc commented Jan 19, 2020

I have the pixel demo nearly running on xs!

But when I try the hostInvite = E(home.gallery).sellToGallery(units2); step, I run into SyntaxError: missing ,. I traced it to an attempt to evaluate function checkUnits (){[native code]}; which is what xs produces from (a bundled version of) this code:

checkUnits: `${escrowExchange.checkUnits}`,

As discussed back in Oct:

XS does not store the source code of functions so Function.prototype.toString always fails.
-- XS Conformance Revised: March 19, 2018

@erights suggested:

We should mark which of our functions actually need their source. ... We could then do a source-to-source transform copying the source of those functions into literal strings we attach to those functions.

So I plan to try something of that sort.

for reference: spec for Function.prototype.toString()

@dckc
Copy link
Member Author

dckc commented Jan 19, 2020

This is a bit ugly, but it seems to work: c58db3e . IOU a PR against master.

I had to bump up the xs parser buffer size: 5c302ea

@erights
Copy link
Member

erights commented Jan 22, 2020

This is a bit ugly, but it seems to work: c58db3e . IOU a PR against master.

This is too ugly, as it mixes directly authored source text with generated text. This is too hard to keep in correspondence. To figure out how to deal with this, let's keep in mind

  • This arises in this form only for contracts based on contractHost, which we are shelving. "Shelving" is a term I made up, to indicate something weaker than deprecating. We expect to rename contractHost to vatSpawner or something like it, and to keep it working as something no longer specific to contracts. We'd move it to tests/ for now to both ensure that it continues to work, and that no one thinks it is yet advertised for general use. The current contractHost-based contracts would move along with it, and so they must continue to work as well.
  • For both contractHost and Zoe (attn @katelynsills @michaelfig) we want to move away from evaluable strings and from rewriters we don't control, and to move to authoring code run under contractHost and Zoe as EcmaScript modules.
  • SES-on-XS doesn't currently provide for runtime loading of new modules. It does support evaluating evaluable strings.

@erights
Copy link
Member

erights commented Jan 22, 2020

Currently, the bundle-source package is how we abstract the dynamic loading of modules. Perhaps contractHost (and Zoe?) needs to be refactored to use it instead. That way, we can easily find all the places we'll need to fix.

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

  • SES-on-XS doesn't currently provide for runtime loading of new modules.

Since I gave you that impression I have learned otherwise; there's an fxLoadScript behind an mxParse compile-time flag. I think that flag is actually on in the platforms that I have been using. I'm not sure why I missed this:

https://github.com/Moddable-OpenSource/moddable/blob/public/xs/sources/xsPlatforms.c#L372-L420

I haven't figured out how to use it yet, and I'm not sure we want to, but the facility does seem to be there.

This point is tangential to this issue; I wonder if there's a better issue or if there should be.

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

This is a bit ugly, but it seems to work: c58db3e .

This is too ugly, as it mixes directly authored source text with generated text. This is too hard to keep in correspondence.

I could agree that the present incarnation is too ugly, but I got the idea of mixing directly authored source text with generated text from you; have you since changed your mind? Or did I misunderstand?

We could then do a source-to-source transform copying the source of those functions into literal strings we attach to those functions. -- @erights Oct 9 https://github.com/Agoric/SES/issues/16#issuecomment-428217371

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

Currently, the bundle-source package is how we abstract the dynamic loading of modules. Perhaps contractHost (and Zoe?) needs to be refactored to use it instead.

The current bundle-source API (a) uses rollup, which I doubt is feasible on xs, and (b) assumes ambient synchronous access to the filesystem. rollup seems to conflict with "move away from evaluable strings and from rewriters we don't control".

fxLoadScript also uses ambient (and synchronous) access to the filesystem, meanwhile. I can think of ways to avoid ambient access (by adding fopen to fxMachine or some such) but I don't see how to avoid synchronous access without a major rewrite.

@erights
Copy link
Member

erights commented Jan 22, 2020

I don't remember what I said, and my apologies if I'm contradicting myself. It would not be the first time that I suggested X, someone did X, and then I complained that X needed to become something else.

Here's a form of source that would be better, and is probably the right short term fix:

Instead of making them unreadable strings, make them template strings with newlines shown as new lines, so the whole thing is human readable.

I don't understand why your discovery is tangential. It seems like it should enable the right long term fix, and could be abstracted by bundle-source as the next step (see #438 ).

@erights
Copy link
Member

erights commented Jan 22, 2020

rollup seems to conflict with "move away from evaluable strings and from rewriters we don't control".

Yes, absolutely. We must stop using rollup or any translator we don't control. We should be able to use https://github.com/Agoric/make-importer to make our rollup replacement for environments (not XS) where we must transform into evaluable strings. Given your discovery, I'm hopeful we can load module more directly on XS.

@erights
Copy link
Member

erights commented Jan 22, 2020

I also forgot that @jfparadis tested and found:

The dynamic import expression works inside a statically loaded module, and dynamically loads new modules into the importing module's Compartment. @jfparadis is that right?

@jfparadis
Copy link
Contributor

Correct. Look at this simple gist:
https://gist.github.com/jfparadis/34245613447ef2371cd6588adc7e83a4#file-import-js

The key file is import.js:

// Allows new Compartment("import").global.import("foo")
globalThis.import = async function(specifier) {
  return import(specifier);
}

I have a more advanced example that I will load in another gist.

@jfparadis
Copy link
Contributor

jfparadis commented Jan 22, 2020

This is the more advanced API:
https://gist.github.com/jfparadis/21c16afae0716781754ac8a980db1db2

The key file is SESCompartment which uses the native XS Compartment as its engine. It is uses like this:

// This shim only support mapping for referrer "*". Although it works, it 
// makes little sense to specify anything else but bare and absolute
// specifiers with referrer "*". 
const cmp = new SESCompartment({}, { '*' : { './mod1': './mod1' } });

// Populate the global before any module is loaded.
cmp.global.g1 = g1Value;

// Load the module.
cmp.import('./mod1');

The key here is the mapping for modules:

const modules = { '*' : { './mod1': './mod1' } }

It goes from referrer to specifier to target which are all strings. It works like this:

  1. the parent creates a child compartment
  2. the parent decides which modules are accessible to the child compartment using a map
  3. the map is a tripple [referrer, specifier, target]`
  4. In the case above, the referrer is '*' which means for all referrers.
  5. The 'specifier: target' pair is './mod1':'./mod1' meaning that the child sees './mod1' as the same module as the parent './mod1'

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

The dynamic import expression works inside a statically loaded module, and dynamically loads new modules into the importing module's Compartment.

new modules? I don't see that yet. I can see that import() is dynamic in that it computes at runtime which module to import, but it has to pick among modules that are already in Compartment.map, no?

@jfparadis
Copy link
Contributor

@dckc Correct, AFAIK.

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

I wonder if import() from the primordial realm can load new modules from disk and can be passed as an endowment to a Compartment and retain the power to load new modules from disk. That would be quite nice: passing a loader as explicit authority.

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

oops... but import() in the primordial realm wouldn't load modules in a pure environment. They could import file and such. Sigh.

@dckc
Copy link
Member Author

dckc commented Jan 22, 2020

Hmm... perhaps we could tweak the xs platform module hooks

@erights
Copy link
Member

erights commented Jan 22, 2020

Could these hooks be reflected back into JavaScript?

Attn @phoddie

@phoddie
Copy link

phoddie commented Jan 22, 2020

I'm not entirely sure I understand the goal here and so hesitant to offer much guidance. The descriptions above are in the context of Agoric work that I don't understand. A statement of the problem from a JavaScript-only perspective might help.

FWIW - you may be able to call back into JavaScript from the fxLoadScript host hook. But... assuming a script is allowed to execute at that time by the spec, doing so seems to have the potential to lead to the kind of engine bugs that Natalie Silvanovich warns about.

@dckc
Copy link
Member Author

dckc commented Jan 23, 2020

I worked out some of the details... XS provides an fxFindModule hook. After primordial realm initialization, we can set a flag so that it doesn't find powerful modules.

https://gist.github.com/dckc/cbbd3e8469723b342cc90799ace7a287

Could these hooks be reflected back into JavaScript?

I'm not sure what you have in mind, but that seems like a lot more trouble than it's worth. Calling JavaScript involves unknown amounts of heap allocation plus longjump() on exceptions. Maybe there's a way to do that safely inside fxLoadScript, but I don't know how.

On the other hand, what I have in mind for setting a flag is a new host function. So in that sense, yes, this flag hook would be reflected back into JavaScript.

@warner warner added the ERTP package: ERTP label Jan 23, 2020
@michaelfig
Copy link
Member

So the latest discussion on these kinds of topics was that we will take baby steps that still have rollup bundling (for CommonJS support). Right now, this means using the "nestedEvaluate" module format, which separates the eval of the module sources into per-module evaluate. We can then provide our own rollup plugin to do our own transform (specifically of ESM) in the future, without losing CommonJS compatibility.

But in all XS cases, we will want to move to bundling most code ahead of time, so there is no rollup at runtime, and then teach SwingSet how to interpret the bundles.

@katelynsills katelynsills added spawner Spawner and contractHost and removed ERTP package: ERTP labels Mar 30, 2020
@katelynsills katelynsills changed the title ERTP ${escrowExchange.checkUnits} idiom not supported by xs ${escrowExchange.checkUnits} idiom not supported by xs Aug 11, 2020
@katelynsills
Copy link
Contributor

Removed mention of ERTP, since that part is stale

@dckc dckc self-assigned this Jan 16, 2021
@dckc dckc added the xsnap the XS execution tool label Apr 28, 2021
@dckc dckc changed the title ${escrowExchange.checkUnits} idiom not supported by xs ${myFunction} idiom not supported by xs Jul 22, 2021
@dckc dckc added the wontfix label Jul 22, 2021
@dckc
Copy link
Member Author

dckc commented Jul 22, 2021

This limitation of XS is not blocking anything any more, AFAICT.

@dckc dckc closed this as completed Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spawner Spawner and contractHost xsnap the XS execution tool
Projects
None yet
Development

No branches or pull requests

7 participants