style(governance): lint for Jessie dialect (WIP) #3876

dckc · 2021-09-22T16:04:07Z

I got surprisingly few lint errors when I added @jessie-check to the governance sources. The code is already remarkably close to Jessie. The biggest issue I see is:

dynamic property names in paramManager

For code such as...

typesAndValues[name] = { type, value };

Is that just irreconcilable with Jessie? Or is there some Object.defineProperty(...) idiom that we prefer in such cases?

The other issue is relatively minor:

use of new in new WeakMap() (1 occurrence)

I'd be tempted to put

export const makeWeakMap = () => new WeakMap();

in the same directory, named tablemaker.js or jessielib.js or some such and import from there. I lean against adding a new package dependency for a 1-liner (plus, I'm not sure how stable our jessie stdlib package is).

Chris-Hibbert · 2021-09-22T16:10:36Z

packages/governance/src/binaryVoteCounter.js

@@ -106,7 +107,7 @@ const makeBinaryVoteCounter = (questionSpec, threshold, instance) => {
      if (choice < 0) {
        spoiled += shares;
      } else {
-        tally[choice] += shares;
+        tally[+choice] += shares;


What does a unary plus do as an array index? Is it coercing choice to a number?

Yes, I think so.

I have only a vague understanding of why, but...

Jessie omits mutation through computed property names. Jessie has syntax for mutating only number-named properties, which include integers, floating point, NaN, Infinity, and -Infinity.
-- https://github.com/endojs/Jessie

We want to make it easier to statically reason more reliably about Jessie programs, while avoiding undue syntactic burden. A typing practice common outside JS and approximated by TS is that records are heterogeneously typed -- each named field is separately typed, and arrays are uniformly typed. But JS fights us here, by making array indexes just be normal property names, and generalizing the [] syntax to apply to all property names. Computed indexing destroys simple static reasoning about heterogeneously typed record fields.

The more correct syntax for Jessie to insist on to ensure computed property names are array indexes would be obj[index|0] as done in asm.js. Perhaps we should do that instead. But we opted for a looser compromise to get a slightly less unpleasant syntax. Our looser compromise depends on normal records not having properties named for floating point values or NaN or Infinity or -Infinity.

(Btw, I think the obj[+index] syntactic compromise is originally due to @mikesamuel . Thanks!)

The

E(paramMgr)[`update${paramName}`]

below is a great example of something that is prohibited by Jessie, but is in this case a good idea. For this one, I think the correct approach is an appropriate eslint-ignore-like comment or ts-ignore-like comment, though I don't yet know how to do those for Jessie. It is the right approach because it is unusual for code like this to be a good idea, and its costs to static reasoning are real and significant. As with ts-ignore comments, the Jessie ignore comment should be accompanied by rationale.

For the sort of formal methods I hope to use, I don't expect them to cope well with ignore comments.
I'm inclined to extract the computed property lookup stuff to a function in a separate file, which we would reason about separately.

Yes. Along the lines of your fromEntries suggestion at #3876 (comment) , for each of these accesses, there is already adequate reflective API. For property reading by itself, for example, there is

Reflect.get(tally, choice)

I had the opposite opinion earlier, but now I don't see many gains from E(paramMgr)[`update${paramName}`]. What makes this a good idea?

The earlier answer was:

each updateFoo() message is a separable capability.

I mentioned "my preference for static typing", which argues for your E(paramMgr).update(paramName, proposedValue) suggestion; it looks like Jessie suggests a similar preference.

I suppose the separate capability could always be expressed using currying: proposedValue => E(paramMgr).update("knob1", proposedValue).

I can see it both ways.

dckc · 2021-09-22T16:27:45Z

Oh... the typesAndValues[name] = { type, value }; is in a loop that constructs typesAndValues. Rewriting to use Object.fromEntries(paramDescriptions.map(...)) seems straightforward.

But typesAndValues is still mutated in the updateFn. Should it be a map rather than an object?

michaelfig · 2021-09-22T16:35:58Z

typesAndValues[name] = { type, value };
Is that just irreconcilable with Jessie? Or is there some Object.defineProperty(...) idiom that we prefer in such cases?

Objects-as-maps is the antipattern we're trying to avoid. Just use Map if possible. If it's impossible, then unfortunately you're out of luck.

I lean against adding a new package dependency for a 1-liner (plus, I'm not sure how stable our jessie stdlib package is).

Please just import { makeWeakMap } from '@jessie.js'. If you've already bought into @jessie-check, there's not any compelling reason to avoid it.

@jessie.js will remain backward-compatible, and we expect to hang other useful things there (maybe even E or a new Far-alike).

erights

Wonderful to see! We need everyone to do a lot more of this! Thanks for getting us started!

erights · 2021-09-22T17:19:18Z

packages/governance/src/binaryVoteCounter.js

@@ -106,7 +107,7 @@ const makeBinaryVoteCounter = (questionSpec, threshold, instance) => {
      if (choice < 0) {
        spoiled += shares;
      } else {
-        tally[choice] += shares;
+        tally[+choice] += shares;


We want to make it easier to statically reason more reliably about Jessie programs, while avoiding undue syntactic burden. A typing practice common outside JS and approximated by TS is that records are heterogeneously typed -- each named field is separately typed, and arrays are uniformly typed. But JS fights us here, by making array indexes just be normal property names, and generalizing the [] syntax to apply to all property names. Computed indexing destroys simple static reasoning about heterogeneously typed record fields.

The more correct syntax for Jessie to insist on to ensure computed property names are array indexes would be obj[index|0] as done in asm.js. Perhaps we should do that instead. But we opted for a looser compromise to get a slightly less unpleasant syntax. Our looser compromise depends on normal records not having properties named for floating point values or NaN or Infinity or -Infinity.

(Btw, I think the obj[+index] syntactic compromise is originally due to @mikesamuel . Thanks!)

erights · 2021-09-22T17:29:07Z

packages/governance/src/binaryVoteCounter.js

@@ -106,7 +107,7 @@ const makeBinaryVoteCounter = (questionSpec, threshold, instance) => {
      if (choice < 0) {
        spoiled += shares;
      } else {
-        tally[choice] += shares;
+        tally[+choice] += shares;


The

E(paramMgr)[`update${paramName}`]

below is a great example of something that is prohibited by Jessie, but is in this case a good idea. For this one, I think the correct approach is an appropriate eslint-ignore-like comment or ts-ignore-like comment, though I don't yet know how to do those for Jessie. It is the right approach because it is unusual for code like this to be a good idea, and its costs to static reasoning are real and significant. As with ts-ignore comments, the Jessie ignore comment should be accompanied by rationale.

katelynsills

This is cool to see! I added comments separately.

One question for @dckc, what's the benefit of export const makeWeakMap = () => new WeakMap();? Maybe I don't understand the reasoning behind wanting to reduce usage of new

dckc · 2021-09-23T02:09:35Z

... for @dckc, what's the benefit of export const makeWeakMap = () => new WeakMap();? Maybe I don't understand the reasoning behind wanting to reduce usage of new

The stated justification is not all that clear to me either:

Arrow functions and concise methods have a [[Call]] behavior and no [[Construct]] behavior, preventing them from being called as a constructor, such as with new. However, Jessie function functions can be called by SES code with new. Without this it is hard to see how this could confuse a function function, but I am not yet confident that this does not produce a hazard for the Jessie code.
-- https://github.com/endojs/Jessie#remaining-hazards

I guess there's a hazard of calling functions as constructors when they weren't meant to be? I'm fuzzy on what that hazzard is @erights maybe you want to beef up the justification a bit? Add a link to some popular explanation of this hazard?

also...

TinySES [and hence Jessie] is not intended to run legacy code or code that uses inheritance.

Jessie doesn't have this nor class and I think it doesn't have prototypical inheritance; I suppose without those, the only use for new is to construct a WeakMap, Map, or Set.. From the perspective of someone writing a Jessie interpreter, adding makeWeakMap library function is in some ways simpler than including the new language construct.

dckc · 2021-10-12T23:06:27Z

@mhofman had a suggestion in the area of types for parameterized governance:

For this to work, GetParams would have to be a generic on the ParamDescription and getLoadParams: GetParams<LoadParamDescriptions>
The getLoanParamValue can then accept a @param {keyof LoadParamDescriptions} key, and typing should flow through

The code is remarkably close to Jessie. Remaining issues include: - new WeakMap() - dynamic property names in paramManager

dckc · 2023-05-06T02:43:28Z

thanks landing a bunch of @jessie-check stuff @erights

dckc requested review from erights, michaelfig, katelynsills and Chris-Hibbert September 22, 2021 16:04

Chris-Hibbert reviewed Sep 22, 2021

View reviewed changes

erights requested changes Sep 22, 2021

View reviewed changes

katelynsills reviewed Sep 23, 2021

View reviewed changes

Chris-Hibbert force-pushed the manageTreasury-3189 branch 7 times, most recently from 97f1d53 to 6ce7f17 Compare October 7, 2021 16:40

Base automatically changed from manageTreasury-3189 to master October 8, 2021 16:32

style(governance): lint for Jessie dialect (WIP)

ed2253f

The code is remarkably close to Jessie. Remaining issues include: - new WeakMap() - dynamic property names in paramManager

dckc force-pushed the dc-treasury-jessie branch from 72b8586 to ed2253f Compare October 14, 2021 15:06

This was referenced Apr 25, 2023

refactor(casting): jessie-check where easy #7515

Merged

refactor(cache): jessie-check where easy #7516

Merged

refactor(agoric-cli): jessie-check where easy #7517

Merged

refactor(internal): jessie-check where easy #7518

Merged

dckc closed this May 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(governance): lint for Jessie dialect (WIP) #3876

style(governance): lint for Jessie dialect (WIP) #3876

dckc commented Sep 22, 2021

Chris-Hibbert Sep 22, 2021

dckc Sep 22, 2021

erights Sep 22, 2021 •

edited

Loading

erights Sep 22, 2021

dckc Sep 22, 2021

erights Sep 22, 2021 •

edited

Loading

katelynsills Sep 23, 2021 •

edited

Loading

dckc Sep 23, 2021

dckc commented Sep 22, 2021

michaelfig commented Sep 22, 2021

erights left a comment •

edited

Loading

erights Sep 22, 2021 •

edited

Loading

erights Sep 22, 2021

katelynsills left a comment

dckc commented Sep 23, 2021

dckc commented Oct 12, 2021

dckc commented May 6, 2023

style(governance): lint for Jessie dialect (WIP) #3876

style(governance): lint for Jessie dialect (WIP) #3876

Conversation

dckc commented Sep 22, 2021

Chris-Hibbert Sep 22, 2021

Choose a reason for hiding this comment

dckc Sep 22, 2021

Choose a reason for hiding this comment

erights Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

erights Sep 22, 2021

Choose a reason for hiding this comment

dckc Sep 22, 2021

Choose a reason for hiding this comment

erights Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

katelynsills Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

dckc Sep 23, 2021

Choose a reason for hiding this comment

dckc commented Sep 22, 2021

michaelfig commented Sep 22, 2021

erights left a comment • edited Loading

Choose a reason for hiding this comment

erights Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

erights Sep 22, 2021

Choose a reason for hiding this comment

katelynsills left a comment

Choose a reason for hiding this comment

dckc commented Sep 23, 2021

dckc commented Oct 12, 2021

dckc commented May 6, 2023

erights Sep 22, 2021 •

edited

Loading

erights Sep 22, 2021 •

edited

Loading

katelynsills Sep 23, 2021 •

edited

Loading

erights left a comment •

edited

Loading

erights Sep 22, 2021 •

edited

Loading