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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/governance/src/binaryVoteCounter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { Far } from '@agoric/marshal';
import { makePromiseKit } from '@agoric/promise-kit';
Expand Down Expand Up @@ -108,7 +109,7 @@ const makeBinaryVoteCounter = (questionSpec, threshold, instance) => {
if (choice < 0) {
spoiled += shares;
} else {
tally[choice] += shares;
tally[+choice] += shares;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@erights erights Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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!)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@erights erights Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@katelynsills katelynsills Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
});

Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/closingRule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

// Standard closing rule is a deadline and Timer. An alternative might allow for
// emergency votes that can close as soon as a quorum or other threshold is
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/committee.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { E } from '@agoric/eventual-send';
import { Far } from '@agoric/marshal';
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/contractGovernor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { E } from '@agoric/eventual-send';
import { Far } from '@agoric/marshal';
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/electorateTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { E } from '@agoric/eventual-send';
import { allComparable } from '@agoric/same-structure';
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/governParam.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { E } from '@agoric/eventual-send';
import { Far } from '@agoric/marshal';
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import './types.js';

Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/question.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { Far, passStyleOf } from '@agoric/marshal';
import { sameStructure } from '@agoric/same-structure';
Expand Down
1 change: 1 addition & 0 deletions packages/governance/src/validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @ts-check
// @jessie-check

import { E } from '@agoric/eventual-send';

Expand Down