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

Improve implementation and type safety of generators #7600

Open
10 tasks
cpcallen opened this issue Oct 20, 2023 · 0 comments
Open
10 tasks

Improve implementation and type safety of generators #7600

cpcallen opened this issue Oct 20, 2023 · 0 comments

Comments

@cpcallen
Copy link
Contributor

cpcallen commented Oct 20, 2023

Description

In the process of converting the generators (generators/*/*) from JavaScript to TypeScript, the strategy for dealing with type errors unearthed by tsc (due to more specific types in the code and/or more assiduous type checking by tsc as compared to Closure Compiler) has been to try to suppress the type errors with casts (as, !, etc.) rather than make other code changes, in an effort to ensure that the .js code generated by tsc is as close as possible to the original—to ensure bug-for-bug compatibility.

Once the TypeScript migration is complete, we should revisit the library blocks implementation and remove such casts, replacing them with:

  • more correct types, where the existing types are not quite correct, and/or
  • type assertions and other runtime checks (rather than casts), and/or
  • bugfixes, where the code is actually wrong.

General issues throughout:

  • Use of ! (typically but not exclusively in the form (!.) to suppress could-be-null errors.
    • Most of these should be changed to explicity check that the value is not null, and throw a helpful error message if the check fails.
    • In the specific case of generator.nameDB_!, we could add a getDistinctName to the Generator class, similar to this one, which would do the check for us so we don't repeat it in every place and don't need to assert on the nameDB
  • Use of as casts (which in some cases may require making the return types of methods in core more specific)
    • Use of as AnyDuringMigration, which covers a multitude of sins including accessing private properties, notably CodeGenerator's .definitions_—about which see issue Provide more user-friendly generator APIs #7326.
    • Use of as OperatorOption, as ConstantOption and as PropertyOption (and those particular types), which are used to suppress type errors when indexing into internal dictionaries without actually checking that the value used as the index is valid. Again these should be explicitly type-checked, probably using a type predicate provided by the block definition for that purpose.
  • Prettier has made a pretty mess out of places where we construct code by repeated concatenation, e.g.:
    -  code += 'for (var ' + loopVar + ' = 0; ' + loopVar + ' < ' + endVar + '; ' +
    -      loopVar + '++) {\n' + branch + '}\n';
    +  code +=
    +    'for (var ' +
    +    loopVar +
    +    ' = 0; ' +
    +    loopVar +
    +    ' < ' +
    +    endVar +
    +    '; ' +
    +    loopVar +
    +    '++) {\n' +
    +    branch +
    +    '}\n';
    The best course of action is probably to use template literals wherever possible.
  • Most generator functions begin with an inline // comment describing the block that they are a generator for. These should be converted to JSDoc /** … */ comments prefixing the export function declaration.
  • See if the various getAdjusted methods on (DartGenerator, JavascriptGenerator and PhpGenerator) can be further improved e.g. by choosing more appropriate types for some of their intermediate values.
  • For historical reasons PythonGenerator provides a getAdjustedInt method instead of a getAdjusted method like the other generators do—and it is the only one of those that actually returns numbers under certain circumstances. This seems gratuitously different, so fix this by:
    • Introducing a new a string-only getAdjusted method.
    • Updating callers to use it (and work correctly when it returns '0' instead of 0).
    • Marking getAdjustedInt as deprecated so that it can be removed in due course.

See Also

Issue #6920 tracks similar work for the library blocks.

Issue #7326 tracks improvements to the CodeGenerator API which should facilitate correcting some of the problems deficiencies outlined above.

@cpcallen cpcallen added issue: bug Describes why the code or behaviour is wrong component: generators issue: triage Issues awaiting triage by a Blockly team member labels Oct 20, 2023
cpcallen added a commit to cpcallen/blockly that referenced this issue Oct 20, 2023
One block protected with // prettier-ignore to preserve careful
comment formatting.

Where there are repeated concatenations prettier has made a pretty
mess of things, but the correct fix is probably to use template
literals instead (rather than just locally disabling prettier).
This has been added to the to-do list in google#7600.
cpcallen added a commit to cpcallen/blockly that referenced this issue Oct 20, 2023
One block protected with // prettier-ignore to preserve careful
comment formatting.

Where there are repeated concatenations prettier has made a pretty
mess of things, but the correct fix is probably to use template
literals instead (rather than just locally disabling prettier).
This has been added to the to-do list in google#7600.
@maribethb maribethb added type: cleanup and removed issue: triage Issues awaiting triage by a Blockly team member issue: bug Describes why the code or behaviour is wrong labels Oct 25, 2023
cpcallen added a commit that referenced this issue Oct 30, 2023
)

* refactor(generators): Migrate javascript_generator.js to TypeScript

* refactor(generators): Simplify getAdjusted

  Slightly simplify the implementation of getAdjusted, in part to
  make it more readable.  Also improve its JSDoc comment.

* refactor(generators): Migrate generators/javascript/* to TypeScript

  First pass doing very mechanistic migration, not attempting to fix
  all the resulting type errors.

* fix(generators): Fix type errors in generator functions

  This consists almost entirely of adding casts, so the code output
  by tsc should be as similar as possible to the pre-migration .js
  source files.

* refactor(generators): Migrate generators/javascript.js to TypeScript

  The way the generator functions are added to
  javascriptGenerator.forBlock has been modified so that incorrect
  generator function signatures will cause tsc to generate a type
  error.

* chore(generator): Format

  One block protected with // prettier-ignore to preserve careful
  comment formatting.

  Where there are repeated concatenations prettier has made a pretty
  mess of things, but the correct fix is probably to use template
  literals instead (rather than just locally disabling prettier).
  This has been added to the to-do list in #7600.

* fix(generators): Fixes for PR #7602

* fix(generators): Fix syntax error
cpcallen added a commit to cpcallen/blockly that referenced this issue Oct 30, 2023
One block protected with // prettier-ignore to preserve careful
comment formatting.

Where there are repeated concatenations prettier has made a pretty
mess of things, but the correct fix is probably to use template
literals instead (rather than just locally disabling prettier).
This is one of the items in the to-do list in google#7600.
cpcallen added a commit that referenced this issue Nov 11, 2023
…7617)

* refactor(generators): Migrate python_generator.js to TypeScript

* refactor(generators): Migrate generators/python/* to TypeScript

  First pass doing very mechanistic migration, not attempting to fix
  all the resulting type errors.

* fix(generators): Fix type errors in generator functions

  This consists almost entirely of adding casts, so the code output
  by tsc should be as similar as possible to the pre-migration .js
  source files.

* refactor(generators): Migrate generators/python.js to TypeScript

  The way the generator functions are added to
  pythonGenerator.forBlock has been modified so that incorrect
  generator function signatures will cause tsc to generate a type
  error.

* chore(generator): Format

  One block protected with // prettier-ignore to preserve careful
  comment formatting.

  Where there are repeated concatenations prettier has made a pretty
  mess of things, but the correct fix is probably to use template
  literals instead (rather than just locally disabling prettier).
  This is one of the items in the to-do list in #7600.
cpcallen added a commit to cpcallen/blockly that referenced this issue Nov 12, 2023
The migration of the JavaScript and Python generators
inadvertently introduced some inconsistencies in the code,
e.g.:

- Incorrect import ordering.
- Putting executable code before the initial comment line that
  most generator functions begin with, and/or deleting or
  replacing these comments.
  -  N.B. however that these inline comments should have been
     JSDocs; a task to convert them has been added to google#7600.
cpcallen added a commit to cpcallen/blockly that referenced this issue Nov 12, 2023
One block protected with // prettier-ignore to preserve careful
comment formatting.

Where there are repeated concatenations prettier has made a pretty
mess of things, but the correct fix is probably to use template
literals instead (rather than just locally disabling prettier).
This is one of the items in the to-do list in google#7600.
cpcallen added a commit to cpcallen/blockly that referenced this issue Nov 13, 2023
The migration of the JavaScript and Python generators
inadvertently introduced some inconsistencies in the code,
e.g. putting executable code before the initial comment line that
most generator functions begin with.  This fixes another instance
of this (but n.b. that these inline comments should have been
JSDocs and a task has been added to google#7600 to convert them).

Additionally, I noticed while doing the PHP migration that
ORDER_OVERRIDES was not typed as specifically as it could be,
in previous migrations, so this is fixed here (along with the
formatting of the associated JSDoc, which can fit on one line
now.)
cpcallen added a commit to cpcallen/blockly that referenced this issue Nov 13, 2023
One block protected with // prettier-ignore to preserve careful
comment formatting.

Where there are repeated concatenations prettier has made a pretty
mess of things, but the correct fix is probably to use template
literals instead (rather than just locally disabling prettier).
This is one of the items in the to-do list in google#7600.
cpcallen added a commit that referenced this issue Nov 14, 2023
* refactor(generators): Migrate dart_generator.js to TypeScript

* refactor(generators): Simplify getAdjusted

  Slightly simplify the implementation of getAdjusted, in part to
  make it more readable.  Also improve its JSDoc comment.

* refactor(generators): Migrate generators/dart/* to TypeScript

  First pass doing very mechanistic migration, not attempting to fix
  all the resulting type errors.

* fix(generators): Fix type errors in generator functions

  This consists almost entirely of adding casts, so the code output
  by tsc should be as similar as possible to the pre-migration .js
  source files.

* fix(generators): Fix minor inconsistencies in JS and Python

  The migration of the JavaScript and Python generators
  inadvertently introduced some inconsistencies in the code,
  e.g.:

  - Incorrect import ordering.
  - Putting executable code before the initial comment line that
    most generator functions begin with, and/or deleting or
    replacing these comments.
    -  N.B. however that these inline comments should have been
       JSDocs; a task to convert them has been added to #7600.

* refactor(generators): Migrate generators/dart.js to TypeScript

  The way the generator functions are added to
  dartGenerator.forBlock has been modified so that incorrect
  generator function signatures will cause tsc to generate a type
  error.

* chore(generator): Format

  One block protected with // prettier-ignore to preserve careful
  comment formatting.

  Where there are repeated concatenations prettier has made a pretty
  mess of things, but the correct fix is probably to use template
  literals instead (rather than just locally disabling prettier).
  This is one of the items in the to-do list in #7600.

* fix(generators): Fix for PR #7646
cpcallen added a commit that referenced this issue Nov 28, 2023
* refactor(generators): Migrate dart_generator.js to TypeScript

* refactor(generators): Simplify getAdjusted

  Slightly simplify the implementation of getAdjusted, in part to
  make it more readable.  Also improve its JSDoc comment.

* refactor(generators): Migrate generators/php/* to TypeScript

  First pass doing very mechanistic migration, not attempting to fix
  all the resulting type errors.

* fix(generators): Fix type errors in generator functions

  This consists almost entirely of adding casts, so the code output
  by tsc should be as similar as possible to the pre-migration .js
  source files.

* fix(generators): Fix more minor inconsistencies in JS and Python

  The migration of the JavaScript and Python generators
  inadvertently introduced some inconsistencies in the code,
  e.g. putting executable code before the initial comment line that
  most generator functions begin with.  This fixes another instance
  of this (but n.b. that these inline comments should have been
  JSDocs and a task has been added to #7600 to convert them).

  Additionally, I noticed while doing the PHP migration that
  ORDER_OVERRIDES was not typed as specifically as it could be,
  in previous migrations, so this is fixed here (along with the
  formatting of the associated JSDoc, which can fit on one line
  now.)

* refactor(generators): Migrate generators/php.js to TypeScript

  The way the generator functions are added to
  phpGenerator.forBlock has been modified so that incorrect
  generator function signatures will cause tsc to generate a type
  error.

* chore(generator): Format

  One block protected with // prettier-ignore to preserve careful
  comment formatting.

  Where there are repeated concatenations prettier has made a pretty
  mess of things, but the correct fix is probably to use template
  literals instead (rather than just locally disabling prettier).
  This is one of the items in the to-do list in #7600.

* docs(generators): @fileoverview -> @file

  With an update to the wording for generators/php.ts.

* fix(generators): Fixes for PR #7647.

  - Don't declare unused wherePascalCase dictionary.
  - Don't allow null in OPERATOR dictionary when not needed.
  - Fix return type (and documentation thereof) of getAdjusted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants