Skip to content

Do not use __ before garbage collection API #1648

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

Closed
aminya opened this issue Jan 30, 2021 · 8 comments
Closed

Do not use __ before garbage collection API #1648

aminya opened this issue Jan 30, 2021 · 8 comments
Labels

Comments

@aminya
Copy link

aminya commented Jan 30, 2021

Copied from #1559 (comment)

Description

I want to propose to use a better notation for the public assembly script functions. There is no need to prepend __ before the end of the functions.

Reasoning

Usually, __ is used before something that should not be used, not something that every developer needs to call manually. TypeScript provides many ways to prevent name conflicting that are better than prefixing underscores.

I would recommend renaming __collect to collect. If someone wants these functions to have different names (rare), they can do:

import { collect as as_collect } from '...'

Using __ and similar notations are used in languages like C that do not have any notion of namespace or module and the names conflict with each other. This is not certainly the case for AssemblyScript.

Example from other languages

Julia, the only language I know that allows triggering garbage collection manually, uses GC.gc() function for triggering garbage collection. GC is a module (namespace) and gc is the function name.

https://docs.julialang.org/en/v1/base/base/#Base.GC.gc

Potential Methods

  1. There was one suggested by @MaxGraey here:
    Rewrite runtime, switch to tracing GC and bootstrap #1559 (comment)

  2. If I was the one who makes the decision, I would just use the power of TypeScript namespaces and modules. In TypeScript it is already possible to rename the functions using as when importing.

@dcodeIO
Copy link
Member

dcodeIO commented Jan 31, 2021

The expectation is that the runtime helpers are temporary and will go away once Wasm GC becomes available. As such I am not in favor of changing their names, since that is an avoidable intermediate breaking change. Also, the current naming scheme reliably avoids name collections in practice, while alternative's do not.

@ghost
Copy link

ghost commented Feb 2, 2021

Namespacing would always prevent the naming collison, afaik, that's quite literially it's sole purpose. How would it not prevent collisions in practice?

@aminya
Copy link
Author

aminya commented Feb 2, 2021

Are namespaces supported by assemblyscript? If that is true, there is no point in using these old workarounds for name collisions.

@ghost
Copy link

ghost commented Feb 2, 2021

@aminya I think so

Btw, another example: when calling V8 internals from JS, one can just call globalThis.gc(true) or %collectGarbage(true), but it's not namespaced, nor is it exposed by default either though.

@dcodeIO
Copy link
Member

dcodeIO commented Feb 2, 2021

Regarding namespacing, this can conflict:

// in library code, implicit
export { gc }; // with gc.collect() for example

// in user code, explicit
export const gc = true; // primitive, cannot have a `.collect()` property

The __ naming is not pretty but more robust. Overall doesn't seem worth to break the API for something that isn't clearly an improvement.

@ghost
Copy link

ghost commented Feb 2, 2021

@dcodeIO That cannot possibly conflict, since the user code has to explicitly import the library's gc variable, it can't end up there on accident.

This, on the other hand would be a problem:

import { gc } from "./library.as";
export const gc = true;

Anyway, one could easily do:

import { gc as garbageCollection } from "./library.as";
export const gc = true;

@dcodeIO
Copy link
Member

dcodeIO commented Feb 2, 2021

When the --exportRuntime option is used, it may, since user code doesn't have to import it and can happily shadow. Providing the option is useful, however, so one doesn't have to bake that into code.

@github-actions
Copy link

github-actions bot commented Jun 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jun 2, 2021
@github-actions github-actions bot closed this as completed Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants