Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Is it possible to get rid of side effects when evaluating api.tx etc? #4994

Closed
btwiuse opened this issue Jun 30, 2022 · 9 comments · Fixed by #5009
Closed

Is it possible to get rid of side effects when evaluating api.tx etc? #4994

btwiuse opened this issue Jun 30, 2022 · 9 comments · Fixed by #5009

Comments

@btwiuse
Copy link
Contributor

btwiuse commented Jun 30, 2022

  • I'm submitting a

    • Other
  • What is the current behavior and expected behavior?

Tab completion is not working in some circumstances.

Pressing tab(s) on the input api.tx. shows nothing:

$ deno repl --eval-file=https://deno.land/x/subshell@0.0.2/tini.ts
api is initializing. Please hold on...
Deno 1.23.1
exit using ctrl+d or close()
> api.tx.

Same is true for api.query., api.consts., api.rpc., etc.

Expected tab completion to work without patching Deno

> api.tx.
system                      session                     claims                      electionProviderMultiPhase  ump                         name                        __defineSetter__
scheduler                   grandpa                     vesting                     voterList                   hrmp                        arguments                   hasOwnProperty
preimage                    imOnline                    utility                     configuration               parasDisputes               caller                      __lookupGetter__
babe                        democracy                   identity                    parasShared                 registrar                   constructor                 __lookupSetter__
timestamp                   council                     proxy                       paraInclusion               slots                       apply                       isPrototypeOf
indices                     technicalCommittee          multisig                    paraInherent                auctions                    bind                        propertyIsEnumerable
balances                    phragmenElection            bounties                    paras                       crowdloan                   call                        valueOf
authorship                  technicalMembership         childBounties               initializer                 xcmPallet                   toString                    toLocaleString
staking
  • What is the motivation for changing the behavior?

To improve the dev experience of using https://deno.land/x/polkadot in Deno repl.

  • Please tell us about your environment:

    • Version: deno.land/x/polkadot@0.0.1

    • Environment:

      • Other: Deno 1.23.1
    • Language:

      • TypeScript

Related: denoland/deno#14967

Adding --v8-flags=--trace-side-effect-free-debug-evaluate shows that evaluating api.tx failed side effect check.

$ deno repl --v8-flags=--trace-side-effect-free-debug-evaluate --eval-file=https://deno.land/x/subshell@0.0.2/tini.ts
api is initializing. Please hold on...
Deno 1.23.1
exit using ctrl+d or close()
> api.tx.[debug-evaluate] Checking function  for side effect.
0x11230843cac1 <BytecodeArray[10]>[debug-evaluate] Checking function get tx for side effect.
0x11230843cb95 <BytecodeArray[13]>[debug-evaluate] Checking function assertResult for side effect.
0x11230843cc79 <BytecodeArray[15]>[debug-evaluate] bytecode LdaModuleVariable may cause side effect.
[debug-evaluate] Function assertResult failed side effect check.
[debug-evaluate] Function assertResult failed side effect check.

Deno repl skips tab completion for expressions evaluated with side effects: https://github.com/denoland/deno/blob/8d82ba729937baf83011354242cabc3d50c13dc2/cli/tools/repl/editor.rs#L132

I searched for similar problems in Deno's issue history, and it seems no one encountered the same tab completion problem before.

My question: why evaluating api.tx introduces side effect and is it possible to get rid of it?

I understand that deno.land/x/polkadot is experimental, and Deno is not currently a supported target, but still would like to play with it more. Feel free to close this if it's off topic.

Thanks.

@jacogr
Copy link
Member

jacogr commented Jun 30, 2022

No idea, but actually saw that issue you logged, aka works in Node doesn't quite work in Deno.

If I had to take a guess... It is quite possibly due to the fact that api.{query, events, errors, tx, runtime ...}.* are all lazily decorated. The methods are decorated by adding getters on the object, see for instance the decoration on tx (the others operate the same).

However... looking at your specific log, it seems to be due to the use of assertResult - not 100% sure these are all needed anymore (it used to), I actually saw that earlier this week and for most of these I do believe the object would never be null or undefined.

So if you are in the mood for a small PR - would suggest just dropping all of these in Getters.ts and re-add if TS does complain about one/two.

@btwiuse
Copy link
Contributor Author

btwiuse commented Jun 30, 2022

Thanks, I'll give it a shot.

BTW how are the files in https://github.com/polkadot-js/build-deno.land generated?

I'd like to first test it locally before submitting a PR.

@jacogr
Copy link
Member

jacogr commented Jun 30, 2022

On build, the deno output is placed in build-deno - it gets copied as-is to the above repo. So for instance, after running yarn build, you will have packages/api/build-deno with the Deno-compatible output. (alongside build for the full thing and build-cjs for the CJS-only output)

@btwiuse btwiuse changed the title Is it possible to rid of side effects when evaluating api.tx etc? Is it possible to get rid of side effects when evaluating api.tx etc? Jun 30, 2022
btwiuse added a commit to subqns/api that referenced this issue Jul 1, 2022
@btwiuse
Copy link
Contributor Author

btwiuse commented Jul 1, 2022

I think I've found something interesting.

If we define assertReturn locally instead of importing it from another module, v8 would think that the getter has no side effect.

Experiment:

Create three files under current directory (bash)

cat <<EOF >is.ts
export function isUndefined(value?: unknown): value is undefined {
  return typeof value === "undefined";
}

export function isFunction(value: unknown): value is Function {
  return typeof value === "function";
}
EOF

cat <<EOF >module.ts
import { isFunction, isUndefined } from "./is.ts";

export function assertReturnModule<T>(
  value: T | undefined,
  message: string | (() => string),
): T {
  if (isUndefined(value)) {
    throw new Error(
      isFunction(message) ? message() : message,
    );
  }
  return value;
}
EOF

cat <<EOF >init.ts
import { isFunction, isUndefined } from "./is.ts";
import { assertReturnModule } from "./module.ts";

export function assertReturnLocal<T>(
  value: T | undefined,
  message: string | (() => string),
): T {
  if (isUndefined(value)) {
    throw new Error(
      isFunction(message) ? message() : message,
    );
  }
  return value;
}

class Getter {
  public name: string | undefined;
  constructor(name?: string) {
    this.name = name;
  }
  public get M(): string {
    return assertReturnModule(this.name, "name cannot be null");
  }
  public get L(): string {
    return assertReturnLocal(this.name, "name cannot be null");
  }
}

let G = new Getter("v8 getter side effect check");
EOF

Then start deno repl with the following flags

$ deno repl --v8-flags=--trace-side-effect-free-debug-evaluate --eval-file=init.ts
Deno 1.23.1
exit using ctrl+d or close()

G.M failed side effect check, with no tab completion:

> G.M.[debug-evaluate] Checking function  for side effect.
0x3b67003ae8e9 <BytecodeArray[10]>[debug-evaluate] Checking function get M for side effect.
0x3b67003ae9e5 <BytecodeArray[18]>[debug-evaluate] Checking function assertReturnModule for side effect.
0x3b67003aea89 <BytecodeArray[47]>[debug-evaluate] bytecode LdaModuleVariable may cause side effect.
[debug-evaluate] Function assertReturnModule failed side effect check.
[debug-evaluate] Function assertReturnModule failed side effect check.

G.L passed, and tab completion is working:

> G.L.[debug-evaluate] Checking function  for side effect.
0x3b67003af019 <BytecodeArray[10]>[debug-evaluate] Checking function get L for side effect.
0x3b67003af0f5 <BytecodeArray[18]>[debug-evaluate] Checking function assertReturnLocal for side effect.
0x3b67003af199 <BytecodeArray[43]>[debug-evaluate] Checking function isUndefined for side effect.
0x3b67003af24d <BytecodeArray[5]>[debug-evaluate] Checking function  for side effect.

length                charAt                fixed                 match                 replaceAll            substr                trimLeft              valueOf               propertyIsEnumerable
constructor           charCodeAt            includes              matchAll              search                substring             trimEnd               __defineGetter__      toLocaleString
anchor                codePointAt           indexOf               normalize             slice                 sup                   trimRight             __defineSetter__
at                    concat                italics               padEnd                small                 startsWith            toLocaleLowerCase     hasOwnProperty
big                   endsWith              lastIndexOf           padStart              split                 toString              toLocaleUpperCase     __lookupGetter__
blink                 fontcolor             link                  repeat                strike                trim                  toLowerCase           __lookupSetter__
bold                  fontsize              localeCompare         replace               sub                   trimStart             toUpperCase           isPrototypeOf
> G.L.

assertReturnModule and assertReturnLocal have the same definition, but are treated differently when converted to v8 bytecode.

It indicates that we can get rid of the LdaModuleVariable bytecode by manually inlining the imported function.

@btwiuse
Copy link
Contributor Author

btwiuse commented Jul 1, 2022

I created #5009 as a workaround, where assertReturn is replaced with equivalent inlined code.

jacogr added a commit that referenced this issue Jul 1, 2022
* Add side effect check workaround (#4994)

* Update packages/api/src/base/Getters.ts

Co-authored-by: Jaco <jacogr@gmail.com>

* Update packages/api/src/base/Getters.ts

Co-authored-by: Jaco <jacogr@gmail.com>

* eslint --fix

* inline isUndefined

Co-authored-by: Jaco <jacogr@gmail.com>
@jacogr
Copy link
Member

jacogr commented Jul 1, 2022

Do you need a version published to Deno? (If you can wait a day or two, it will happen Saturday/Sunday with a new published API - if absolutely urgent, can do one manually)

@btwiuse
Copy link
Contributor Author

btwiuse commented Jul 1, 2022

Do you need a version published to Deno? (If you can wait a day or two, it will happen Saturday/Sunday with a new published API - if absolutely urgent, can do one manually)

Absolutely not urgent, but I'd like to see it land asap if that's ok.

image

I'm too excited to wait :)

@jacogr
Copy link
Member

jacogr commented Jul 2, 2022

https://github.com/polkadot-js/build-deno.land/releases/tag/0.0.2

@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Jul 9, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants