Skip to content

Conversation

IamIpanda
Copy link
Contributor

Fix missing let decs inner declaration.
I'm not sure if it's needed to fix that breaking return let which won't happen on auto Var as autovar move all variables to top.

@edemaine
Copy link
Collaborator

edemaine commented Jan 3, 2023

If we want autoLet to be a viable option, I think we should fix the return let...

@IamIpanda
Copy link
Contributor Author

IamIpanda commented Jan 4, 2023

If we want autoLet to be a viable option, I think we should fix the return let...

Return let can be fixed, but it would be only the start as let don’t return value, that means any assignment as an expression should be checked, and hoist to a proper position (Should be nearest position where can put a let, or maybe just block top to make it possible.) I’m not sure if now AST support to detect the AssignmentExpression’s return value is “used”

I’m checking the PR similar to that for coffeescript: jashkenas/coffeescript#5395

@STRd6
Copy link
Contributor

STRd6 commented Jan 4, 2023

Here's an idea:

When a declaration is in return position prepend it with a simple let ref, then assign the final declaration to the ref, then return the ref.

->
  let a = 1, b = 2
---
function() {
  let ref
  let a, b = ref = 2
  return ref
}

This may be useful for Civet in general to have declarations behave more like expressions.

@edemaine
Copy link
Collaborator

edemaine commented Jan 4, 2023

Nice, yes, maybe we can use this to allow let declarations within if..else expressions (along with return). But it doesn't work with const... An alternative is to wrap in IFFEs, like for and do. This seems more universal. Hmm, except then it's hard to return out of the block (e.g. if x then return let y = 5).

@IamIpanda
Copy link
Contributor Author

I think it would be quite normal for following code before how it interact with implicit return

a = b = 3
c = (d = 3)
export default MyReactFC = () => <></>

@edemaine
Copy link
Collaborator

edemaine commented Jan 4, 2023

Anyway, this PR is probably worth merging already, as it fixes some bugs in autoLet (right? or am I misunderstanding?). Perhaps should add a comment to the return let example like // TODO This is currently invalid. And we can continue to improve inner declarations in future PRs.

@IamIpanda
Copy link
Contributor Author

Comment added.

@edemaine
Copy link
Collaborator

edemaine commented Jan 4, 2023

c = (d = 3)

Right, this currently compiles to let c = (let d = 3) which is invalid. So it's an issue in many different contexts, though more of an issue for autoLet. For now it could be a "beware" for autoLet users, but it would be nice to fix it in the way Daniel suggests. @IamIpanda are you willing to try working on this? Presumably whenever the added let isn't immediately inside a code block, the let needs to get hoisted to that code block.

@STRd6
Copy link
Contributor

STRd6 commented Jan 4, 2023

autoVar does hoisting to the enclosing function blocks so a similar mechanism could be used here as well.

@STRd6 STRd6 merged commit c4569e6 into DanielXMoore:master Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants