Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
I believe this is the script that YJ noticed it in the first place, so it makes sense this has updated.
There was a problem hiding this comment.
the riddle small input is
ANSWER = 41803
fn t(@s) {
return (ANSWER * s + 12345) % 214748
}
xs = 205804
ys = 71816
ox = 35 - (t(xs) % 70)
oy = 35 - (t(ys) % 70)
r = startSketchOn(XZ)
|> startProfile(at = [ox, oy])
|> line(end = [1, 0])
|> line(end = [0, -1])
|> line(end = [-1, 0])
|> close(%)
|> extrude(length = 1)
And the answer inside the function did not get converted before, So this is also fixing an existing bug
| fn rename_identifiers(&mut self, old_name: &str, new_name: &str, excluded: &[&str]) { | ||
| let param_names: Vec<&str> = self.params.iter().map(|p| p.identifier.name.as_str()).collect(); | ||
| let excluded_for_body: Vec<&str> = excluded.iter().copied().chain(param_names.iter().copied()).collect(); | ||
| self.body.rename_identifiers(old_name, new_name, &excluded_for_body); |
There was a problem hiding this comment.
Is there a reason this couldn't be simplified to this, without needing excluded? Now that I'm writing this, I'm realizing... what happens if they're trying to rename to the same name as a parameter? That will be broken. But it's better than what we have now.
| self.body.rename_identifiers(old_name, new_name, &excluded_for_body); | |
| if !self.params.contains(old_name) { | |
| self.body.rename_identifiers(old_name, new_name); | |
| } |
There was a problem hiding this comment.
I have changed again since see #10117 (comment)
And yeah I think we always need to recurse because it's not only looking for params.
There was a problem hiding this comment.
Sorry, maybe I'm a little slow. But once we find that a name has been shadowed, why do we need to continue? Can't we stop there? We're not trying to do some kind of bulk rename of multiple things, where some things are shadowed and some aren't. Either the old_name is not re-bound and so we need to continue, or it is re-bound and so we're done. In the case of a local var being defined, we can break out of the loop of body items when we see old_name being redefined.
| let refs: Vec<&str> = excluded_owned.iter().map(String::as_str).collect(); | ||
| item.rename_identifiers(old_name, new_name, &refs); | ||
| if let BodyItem::VariableDeclaration(vd) = item { | ||
| excluded_owned.push(vd.declaration.id.name.clone()); |
There was a problem hiding this comment.
If we really want to be correct, it's a bit more complicated than this. Expressions can introduce name bindings. TagDeclarators are the obvious one, but there are others.
For the frontend refactoring, I made a function that finds all bindings in an expression.
But also... I don't think it's necessary to handle all these edge cases in this PR if it's complicated. What you have is already so much better than what's in main. OTOH, I suppose there's an argument that if we can't be correct, main not renaming enough is maybe better than renaming too much.
There was a problem hiding this comment.
Still not sure if I'm covering everything.
675e327
There was a problem hiding this comment.
Here's a test that seems to fail on your branch. Rename creates an error. Let me know if you think this is too obscure. But this is the recursive nature of the beast.
BEST = 2
fn foo() {
sketch001 = startSketchOn(XY)
profile001 = startProfile(sketch001, at = [0, 0])
|> xLine(length = BEST)
|> yLine(length = BEST, tag = $BEST)
|> line(endAbsolute = [profileStartX(%), profileStartY(%)])
|> close()
return profile001
}
foo()…s, named fns) - In function bodies, exclude a name only for items after the one that binds it, so use-before-local still renames (refers to global); bindings in the current item are excluded while processing it so tags/labels/named fns aren’t renamed. - Add body_item_defined_names and collect_defined_names_expr/binary_part (mirror frontend find_defined_names) so tags, LabelledExpression labels, and optional function names are treated as bindings; VariableDeclaration still only excludes init bindings for the current item (not the decl id) so RHS sees the global. - Make TagDeclarator respect excluded when renaming. - Add tests: tag binding not renamed, named function expression not renamed.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| /// Rename all identifiers that have the old name to the new given name. | ||
| fn rename_identifiers(&mut self, old_name: &str, new_name: &str) { | ||
| fn rename_identifiers(&mut self, old_name: &str, new_name: &str, excluded: &[&str]) { | ||
| self.callee.rename(old_name, new_name); |
There was a problem hiding this comment.
The callee identifier is renamed without checking the excluded list. This will incorrectly rename function calls where the callee is a shadowed identifier (e.g., a function parameter or local variable with the same name as a global being renamed).
Impact: When renaming a global like FUNC, a call like FUNC() where FUNC is actually a parameter will be incorrectly renamed, breaking the code.
Fix:
if !excluded.contains(&self.callee.name.as_str()) {
self.callee.rename(old_name, new_name);
}This matches the pattern used for Expr::Name (lines 1356-1360) and BinaryPart::Name (lines 1851-1855).
| self.callee.rename(old_name, new_name); | |
| if !excluded.contains(&self.callee.name.as_str()) { | |
| self.callee.rename(old_name, new_name); | |
| } |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Resolves #10114. Resolves #7751.