Skip to content

Conversation

fangyi-zhou
Copy link
Contributor

@fangyi-zhou fangyi-zhou commented Jul 12, 2025

Closes #15, #248

This change adds additional checks when building bindings, so that
variables that are used before initialised are correctly flagged as
errors.

Currently, we error on uninitialised variables by checking their flow
style. If the flow style is uninitialised or partially uninitialised, an
error is generated. When no flow style information is obtainable (which
is the case for use before write), a default "other" flow style value is
returned.

In this diff, we perform additional checks before returning a default
"other" value. By looking up the scopes for static information for
variables, we can correctly detect if a variable is declared in an upper
scope or in the current scope, and return "uninitialised" flow style
when the variable is used before write.

When doing so, we need to consider the special case for type usages,
so that name resolutions in types do not fail.

Help needed:
There's a failing test that I need some help: test::flow::test_assert_not_in_flow

testcase!(
    test_assert_not_in_flow,
    r#"
from typing import assert_type, Literal
if 0.1:
    vari = "test"
    raise SystemExit
assert_type(vari, Literal["test"])   <-- Shall we raise an error here?
"#,
);

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me, thanks for the PR!

There are a lot of test failures I can't explain easily - I wonder if you just need a rebase? I see compile errors on dataclasses which doesn't seem to be changed here so that's my best guess

After a rebase, if you publish I could work on getting it merged

@fangyi-zhou fangyi-zhou force-pushed the additional-scope-check branch from 5b238ae to d2ee61a Compare July 16, 2025 20:33
@fangyi-zhou fangyi-zhou marked this pull request as ready for review July 16, 2025 20:34
@fangyi-zhou
Copy link
Contributor Author

Rebased and addressed your inline comments. There should be only one test failing (which is the one I linked in the PR description)

@yangdanny97
Copy link
Contributor

Thanks!

For the failing test case test_assert_not_in_flow, I think raising an error there would be correct.

@fangyi-zhou
Copy link
Contributor Author

Thanks!

For the failing test case test_assert_not_in_flow, I think raising an error there would be correct.

If it's not too inconvenient, could you fix that for me during the import? Thanks!

@facebook-github-bot
Copy link
Contributor

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D78496802.

@stroxler
Copy link
Contributor

This may have to wait a day or two, we have some internal infra issues and I can't import the PR right now

@fangyi-zhou fangyi-zhou force-pushed the additional-scope-check branch from d2ee61a to 21680eb Compare July 17, 2025 22:12
Closes facebook#15, facebook#248

This change adds additional checks when building bindings, so that
variables that are used before initialised are correctly flagged as
errors.

Currently, we error on uninitialised variables by checking their flow
style. If the flow style is uninitialised or partially uninitialised, an
error is generated. When no flow style information is obtainable (which
is the case for use before write), a default "other" flow style value is
returned.

In this diff, we perform additional checks before returning a default
"other" value. By looking up the scopes for static information for
variables, we can correctly detect if a variable is declared in an upper
scope or in the current scope, and return "uninitialised" flow style
when the variable is used before write.

When doing so, we need to consider the special case for type usages,
so that name resolutions in types do not fail.
@fangyi-zhou fangyi-zhou force-pushed the additional-scope-check branch from df0357f to 9cbd480 Compare July 18, 2025 00:12
@facebook-github-bot
Copy link
Contributor

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D78496802.

@facebook-github-bot
Copy link
Contributor

@stroxler has imported this pull request. If you are a Meta employee, you can view this in D78496802.

Copy link
Contributor

@stroxler stroxler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I'll work on getting it merged. Thanks!

@fangyi-zhou
Copy link
Contributor Author

Thanks a lot! @stroxler

@facebook-github-bot
Copy link
Contributor

@stroxler merged this pull request in 600c8f1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uninitialized locals check
4 participants