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

Make devices and presences lack property names #793

Closed
wants to merge 2 commits into from

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Mar 27, 2020

Closes #792

This PR improves the debuggability and predictability of device nodes and presences created by SwingSet and CapTP. Adding a Symbol.for('SwingSet') property (which doesn't show up with Object.getOwnPropertyNames), and setting a prototype that defines toString() (which allows the presence to be stringified as before) means that empty objects are marshalled compatibly as empty presences.

The REPL is improved to sniff specifically for this mechanism to distinguish presences from empty objects.

@michaelfig michaelfig added captp package: captp SwingSet package: SwingSet cosmic-swingset package: cosmic-swingset labels Mar 27, 2020
@michaelfig michaelfig requested review from warner and erights March 27, 2020 05:37
@warner
Copy link
Member

warner commented Mar 27, 2020

Ugh, tap is starting to annoy me.. it's really hard to find where the test failure is.

Looks like it's in SwingSet, in test-marshal.js. I'm guessing we have a test that expects .toString to be present.

@michaelfig michaelfig force-pushed the mfig/792-devices-presences-no-names branch 8 times, most recently from 6c1bbb7 to c5b033f Compare March 27, 2020 18:33
Even if they are marshalled as presences, a presence should have no property names or symbols.

Still allow identification by setting a prototype with a toString() method and a [Symbol.for('SwingSet')] property.
@michaelfig michaelfig force-pushed the mfig/792-devices-presences-no-names branch from c5b033f to 8700aba Compare March 27, 2020 18:54
@michaelfig
Copy link
Member Author

I've updated the approach: the current idea is to represent presences as an empty object with a prototype that defines:

{
  toString() { ... },
  [Symbol.for('SwingSet')]: true,
  [Symbol.toStringTag]: description,
}

This allows for simple identification of the objects in the REPL, as well as informative printing and stringification.

@michaelfig
Copy link
Member Author

michaelfig commented Mar 27, 2020

For example:

> const o = ...made as described;
> o
Object [Presence 52] {}
> String(o)
'[Presence 52]'
> throw o
Uncaught Object [Presence 52] {}
> Object.getOwnPropertyNames(o)
[]
> Object.getOwnPropertySymbols(o)
[]
>

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

If we're going to implement #792, this is a reasonable way to do it. But please get @erights 's approval on the overall approach before landing, he's got a better sense of what sorts of promise we ought to be making than I do.

packages/SwingSet/src/kernel/liveSlots.js Outdated Show resolved Hide resolved
@michaelfig michaelfig force-pushed the mfig/792-devices-presences-no-names branch from 7479b91 to ad662c8 Compare March 28, 2020 04:40
@erights
Copy link
Member

erights commented Apr 25, 2020

Is this subsumed by #804 ?

@michaelfig
Copy link
Member Author

Is this subsumed by #804 ?

Yes.

@michaelfig michaelfig closed this Apr 25, 2020
@michaelfig michaelfig deleted the mfig/792-devices-presences-no-names branch August 23, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
captp package: captp cosmic-swingset package: cosmic-swingset SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Presences should not have ownProperties
3 participants