Skip to content

[Kernel] VariableDeclaration should have more properties to help clients #51554

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

Open
rakudrama opened this issue Feb 28, 2023 · 6 comments
Open
Assignees
Labels
front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P3 A lower priority bug or feature request

Comments

@rakudrama
Copy link
Member

It would be helpful if VariableDeclaration had more properties to capture common questions from the client.

What is it?

  • Is the variable a user-defined variable or a synthesized variable
    • Needed for improved debugging experience
    • Useful for prioritizing 'nice' names in non-minified code
  • Is the variable a parameter?
    • VariableDeclaration has properties that pertain only to parameters. Could VariableDeclaration and (new) ParameterDeclaration be subclasses of a common class?

How is it used?

The following summary properties would be useful to transformers and back-ends.
These properties would be much more useful if updated automatically when a transform changes the usage.

  • Most parameters and many variables are single-definition-multiple-use even if not declared final.
  • Some variables are closed-over by local functions or closures. Which variables have dynamic extent due to this?

/cc @fishythefish @annagrin

@rakudrama rakudrama added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel labels Feb 28, 2023
@johnniwinther
Copy link
Member

cc @alexmarkov @nshahan

@johnniwinther johnniwinther self-assigned this Mar 1, 2023
@alexmarkov
Copy link
Contributor

VM also needs a way to distinguish synthetic variables in order to hide them in the debugger (#51512).

I see the following ways of doing that:

  1. Remove names from all synthetic variables. Currently, front-end sometimes generates synthetic variables without names, and VM recognizes those and hides them. We can probably do that for all synthetic variables. The disadvantage of this approach is that we would not be able to give more readable names for those synthetic variables, which can be useful when examining kernel. Also, I heard that at some point VM needed a variable name if variable is captured. This limitation should be fixed if it still exists and we take this approach.

  2. Adopt a fixed and unified naming scheme for synthetic variables. For example, we can make sure that names of synthetic variables always start with #. Front-end could provide helper functions to generate such names and use them uniformly. VariableDeclaration.isSynthetic predicate can be implemented by looking at the name, and VM can be also modified to recognize names starting with #. The advantage of this approach is that we would be able to give readable names to synthetic variables, but this may need a thorough sweep over front-end to make sure helper functions for generating synthetic names are used everywhere.

  3. Add isSynthetic flag to VariableDeclaration. This is probably the most flexible approach, but probably more complex, as we would need to propagate this flag in the front-end, kernel and in the VM from kernel through compiler to the debugger. Variable names are already passed so solutions based on names don't need this extra handling.

@alexmarkov alexmarkov added this to the Dart 3 beta 3 milestone Mar 1, 2023
@alexmarkov alexmarkov added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Mar 1, 2023
@annagrin
Copy link
Contributor

annagrin commented Mar 1, 2023

DDC needs this as well of course. We need to identify variables that are generated by the CFE and do not need to be shown in the debugger. Either adding a kind to the VariableDeclaration or marking it as synthetic will work for us.

Here is an example of what currently shows in the debugger on web platform for patterns: dart-lang/webdev#1974

/cc @nshahan @Markzipan

copybara-service bot pushed a commit that referenced this issue Mar 10, 2023
This adds an [isSynthesized] flag to the [VariableDeclaration] the
signal when the variable doesn't correspond to a variable in the
source code.

The name of a variable can only be `null` if it is synthesized.

Partially in response to
#51554

TEST=existing

Change-Id: I94591971f11da09d210c8b25a2d05e22ca05dc62
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/286961
Reviewed-by: Joshua Litt <[email protected]>
Commit-Queue: Johnni Winther <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
@annagrin
Copy link
Contributor

Thanks @johnniwinther! I am able to replace CFE-synthesized names by temporaries now and most of them now don't show in the debugger.

There is one case that still looks code-unrelated, an I think is related to constant propagation changing names in cases in

variables[i].name = '${variables[i].name}${"#$i"}';

Those replaced names are not marked as synthesized and show in the debugger without real connection to the code:

image

It looks like the new names are replacing a and n in the second case:

image

copybara-service bot pushed a commit that referenced this issue Mar 20, 2023
Addresses #51554 (comment)

Closes #51391

TEST=existing

Change-Id: Idec105dcfde4a91d0a21a1907777d6c07e5f01b2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/289224
Commit-Queue: Johnni Winther <[email protected]>
Reviewed-by: Chloe Stefantsova <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
copybara-service bot pushed a commit that referenced this issue Mar 23, 2023
- Update pattern scope tests after CFE switch improvements.
- Fix incorrect end offset detection in expression compiler.
  Account for synthetic variables that can have offsets from
  later code (i.e. created for hoisting).

Related: #51554

Closes: #51825
Change-Id: I07cd319f8996acae2dada96ba6e43eac9e04fbb6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/290706
Reviewed-by: Nicholas Shahan <[email protected]>
Commit-Queue: Anna Gringauze <[email protected]>
Reviewed-by: Mark Zhou <[email protected]>
@itsjustkevin
Copy link
Contributor

@johnniwinther is there work left to be completed here?

@johnniwinther johnniwinther added P3 A lower priority bug or feature request and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Apr 4, 2023
@johnniwinther
Copy link
Member

Moved to post release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

5 participants