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.

Sign up for GitHub

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

Jump to bottom

Add a configuration to ignore the case when x == y return missing? #542

Closed
aviatesk opened this issue Jun 24, 2023 · 1 comment · Fixed by #543
Closed

Add a configuration to ignore the case when x == y return missing? #542

aviatesk opened this issue Jun 24, 2023 · 1 comment · Fixed by #543
Labels
abstract interpretation Improvements to abstract interpretation

Comments

@aviatesk
Copy link
Owner

The possibility of x == y returning Missing when the types of x or y can't be precisely inferred has bothered me for a while. Ignoring this likelihood might compromise JET's accuracy and prevent it from reliably analyzing code that actually employs missing. So, I'm hesitant to make this the default setting - that's my stance. However, I don't wish to impose my views on others, compelling them to understand it and act on them by making unnecessary code changes or annotations. Perhaps, we could provide a configuration that allows for ignoring the possibility that x == y might return missing. Such an approach might complicate things due to the increasing number of configurations, and new issues might arise. However, having such a possibility might be better than nothing. For example, we could deactivate this configuration in interactive instances like @report_call or @test_call since the types of x or y can be easily inferred (or ought to be), and activate it in situations like in report_package where the types of arguments aren't specifically provided. By doing so, we can potentially allow users to be less aware of this configuration's existence.

Originally posted by @aviatesk in JuliaGraphs/Graphs.jl#249 (comment)
@aviatesk aviatesk added the abstract interpretation Improvements to abstract interpretation label Jun 24, 2023
@gdalle
Copy link

gdalle commented Jun 24, 2023 •
edited

Ignoring this likelihood might compromise JET's accuracy and prevent it from reliably analyzing code that actually employs missing.

I agree, it would be setting a dangerous precedent. Plus I assume there are tons of other situations where a function returns what we want 99% of the time, and yet it's still good to catch the 1%

For example, we could deactivate this configuration in interactive instances like @report_call or @test_call since the types of x or y can be easily inferred (or ought to be)

In the discussion about Graphs.jl that sparked this issue, I don't even think the Missing error showed up when I used @report_call. It really is due to abstract interpretation

aviatesk added a commit that referenced this issue Jun 25, 2023
@aviatesk
ignore x::Any == y::Concrete potentially returning missing within…
794c328 
… `analyze_from_definition`

In the `analyze_from_definitions` mode, the analysis begins with the
method signature. However, these signatures can often be abstract,
leading to situations where, in the case of `x == y`, one side might be
concretely inferred, while the other side is not. This can lead to
scenarios where the analysis might proceed while considering the
possibility that `x == y` could return `missing`, and subsequently
result in an error report, which, as discussed in #542, is usually an
undesired false positive.

`analyze_from_definitions` was initially designed as an entry point for
easy analysis, even at the cost of accuracy. Therefore, it might be
better to simply widen the inferred return type
`(x == y)::Union{Bool,Missing}` to `::Any`, thereby reducing potential
errors. This should not apply to interactive entry points where the
input concrete argument types are given at the analysis starting point.
In these cases, the possibility of `missing` being returned should still
be considered.

A bit off-topic, but while making this change, I realized that the
current design of `ReportPass` might not be functioning as effectively
as hoped. Originally, `ReportPass` was meant to allow users well-versed
in JET internals to completely ignore specific reports, thereby deleting
associated calculations (which can not be achieved by filtering out
reports after the analysis has been completed). However, there seems to
be a lack of such advanced users at present, and this is inadvertently
increasing the complexity of the code base, potentially discouraging
potential contributors. It might be beneficial to shift towards a
simpler configuration style that can control specific behaviors of the
analysis with in the short term.

fix #542
@aviatesk aviatesk mentioned this issue Jun 25, 2023
Merged
aviatesk added a commit that referenced this issue Jun 25, 2023
@aviatesk
ignore x::Any == y::Concrete potentially returning missing within…
c466a1c 
… `analyze_from_definition` (#543)

In the `analyze_from_definitions` mode, the analysis begins with the
method signature. However, these signatures can often be abstract,
leading to situations where, in the case of `x == y`, one side might be
concretely inferred, while the other side is not. This can lead to
scenarios where the analysis might proceed while considering the
possibility that `x == y` could return `missing`, and subsequently
result in an error report, which, as discussed in #542, is usually an
undesired false positive.

`analyze_from_definitions` was initially designed as an entry point for
easy analysis, even at the cost of accuracy. Therefore, it might be
better to simply widen the inferred return type
`(x == y)::Union{Bool,Missing}` to `::Any`, thereby reducing potential
errors. This should not apply to interactive entry points where the
input concrete argument types are given at the analysis starting point.
In these cases, the possibility of `missing` being returned should still
be considered.

A bit off-topic, but while making this change, I realized that the
current design of `ReportPass` might not be functioning as effectively
as hoped. Originally, `ReportPass` was meant to allow users well-versed
in JET internals to completely ignore specific reports, thereby deleting
associated calculations (which can not be achieved by filtering out
reports after the analysis has been completed). However, there seems to
be a lack of such advanced users at present, and this is inadvertently
increasing the complexity of the code base, potentially discouraging
potential contributors. It might be beneficial to shift towards a
simpler configuration style that can control specific behaviors of the
analysis with in the short term.

fix #542
@aviatesk aviatesk mentioned this issue Jun 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
abstract interpretation Improvements to abstract interpretation
Projects
None yet
Milestone
No milestone
2 participants
@gdalle @aviatesk