Fix doubled named arguments bug #6422
Conversation
vitvakatu
added
the
CI: No changelog needed
vitvakatu
requested review from
MichaelMauderer,
wdanilo,
farmaazon,
mwu-tow and
kazcw
as code owners
| // We are iterating over the old span tree, while modifying an expression that | ||
| // constantly changes. The assumption here is that as we go, we only modify AST | ||
| // tree deeper than we will look in future iterations. That way we can be sure | ||
| // that the "outer layers" of AST still corresponds to the original span-tree. | ||
| // It is done that way, because we have no way of actually reconstructing the | ||
| // span-tree to be entirely correct on every step. The new AST is after all | ||
| // wrong and needs to be fixed for a reason. Rebuilding the span-tree before | ||
| // replacing named arguments will give you wrong assignments on everything. | ||
| // The loop propagates up the old span tree, knowing that the outer layers of | ||
| // the AST will still corresponds to it. |
There was a problem hiding this comment.
I must admit, I have a strong difficulty in understanding this paragraph:
We are iterating over the old span tree, while modifying an expression that constantly changes
I suppose the expression is actually an ast variable?
The new AST is after all wrong and needs to be fixed for a reason.
What does it mean, "AST" is wrong? What AST? The old one, or that after applying a change?
Rebuilding the span-tree before replacing named arguments will give you wrong assignments on everything.
What is "replacing named arguments"? Looking at the code, I thought we actually are adding new named arguments, or rather: replace the named with the unnamed.
I guess this just to say we change the arguments to named starting from the last one (as it's in the deepest level)? I think such introductory sentence would help me a lot.
There was a problem hiding this comment.
Updated the paragraph, I hope it is better now.
I suppose the expression is actually an ast variable?
Yes
What does it mean, "AST" is wrong? What AST? The old one, or that after applying a change?
Sorry, it was a mistake. The span tree is meant here, not the AST.
What is "replacing named arguments"?
I removed this sentence. The better word is rewriting, though. We rewrite named arguments to a different form.
I guess this just to say we change the arguments to named starting from the last one (as it's in the deepest level)? I think such introductory sentence would help me a lot.
This is actually the opposite. The last argument is on the top of the tree, see attached ASCII graphics. I added an additional note about it.
|
QA: Looks good to me! I've hit this bug a couple of times in the past, so it's great to see it fixed! |
* develop: (34 commits) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496) Finishing Vector Editor (#6470) Proper handling of multiple list views. (#6461) Fix disappearing cached shapes (#6458) Add Execution Context control to Text.write (#6459) Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443) Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442) Hide profile button behind a feature flag (#6430) ...
* develop: Fix cut-off in text visualisations (#6421) Infer correct synthetic name for nested modules (#6525) Delete unused websocket dependency (#6535) Run typecheck and eslint on `./run lint` (#6314) Force pending saves if client closes abruptly (#6514) Continued Execution Context work and some little fixes (#6506) IDE's logging to a file (#6478) Fix application config (#6513) Cloud/desktop mode switcher (#6448) Fix doubled named arguments bug (#6422) Reimplement `enso_project` as a proper builtin (#6352) Fix layer ordering between dropdown and breadcrumbs backgrounds. (#6483) Multiflavor layers (#6477) DataflowAnalysis preserves dependencies order (#6493) Implement `create_database_table` for Database Table (#6467) Limit Dead Letter logging (#6482) More reliable shutdown of the EnsoContext to save resources (#6468) Make execution mode `live` default for CLI (#6496)
Pull Request Description
Fixes #6228
An annoying issue was hidden deep in the code for updating named arguments after input port disconnection.
Kudos to @Frizi for help with investigating it and the initial implementation of the fix.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.