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

Automatically promote nonlocal as needed #2513

Merged
merged 3 commits into from
Oct 8, 2023
Merged

Conversation

scauligi
Copy link
Member

Promote nonlocal declarations to global ones if the "nearest" matching definition is in the global scope.

Also addresses #2507; can be considered a fix if we decide that the explicit-nonlocal formulation is the proper way to do things.

@Kodiologist
Copy link
Member

Nice branch name. Although my favorite is still one I saw for a PR to Cataclysm DDA, the-gods-outlawed-burnt-water-long-ago.

@Kodiologist
Copy link
Member

Kodiologist commented Oct 1, 2023

Looks good overall. I agree that this closes #2507.

Let's update the documenation of nonlocal in api.rst.

Can you add a comment or docstring to OuterVar or ResolveOuterVars explaining that their purpose is to decide when to promote nonlocal to global?

Shouldn't OuterVar.__init__ call super().__init__? Even if ast.stmt.__init__ doesn't do anything right now, it might in the future. If super().__init__ actually won't work with what you need to do here, it's worth a comment saying so.

I think scope.parent is not None can be just scope.parent.

for name in has:
    if name in undefined:
        undefined.remove(name)

This is better written undefined = [name for name in undefined if name not in has].

Tests of nonlocal with let are probably better put in tests/native_tests/let.hy, next to the other let-with-nonlocal tests, than in your new test file nonlocal.hy.

ration-log isn't used after it's assigned. It would be better to only return health.

@Kodiologist Kodiologist linked an issue Oct 2, 2023 that may be closed by this pull request
@scauligi
Copy link
Member Author

scauligi commented Oct 3, 2023

Done! I also removed one of the new tests that wasn't actually using nonlocal, as it was redundant with tests already in native_tests/let.hy.

@Kodiologist
Copy link
Member

Thanks. I wouldn't remove mention of what nonlocal compiles to; just say that it can compile to global or nonlocal statements (with links to the Python keywords), and explain in what circumstances it will produce global instead of nonlocal.

@Kodiologist
Copy link
Member

My thinking is that the Hy documentation should always link to the relevant parts of the Python documentation for describing semantics. We want to neither recapitulate Python's documentation, nor leave obvious semantic issues undefined.

@scauligi
Copy link
Member Author

scauligi commented Oct 7, 2023

Alright, updated the nonlocal doc 👍

@Kodiologist Kodiologist merged commit d56af20 into hylang:master Oct 8, 2023
9 checks passed
@scauligi scauligi deleted the kingme branch October 8, 2023 16:40
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.

let, lambda and scope of bindings
2 participants