JETAnalyzer: ignore x::Any == y::Concrete potentially returning missing within analyze_from_definition
#543
Conversation
… `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
changed the title
x::Any == y::Concrete potentially returning missing within analyze_from_definitionx::Any == y::Concrete potentially returning missing within analyze_from_definition
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
=======================================
Coverage 89.81% 89.81%
=======================================
Files 10 10
Lines 3053 3055 +2
=======================================
+ Hits 2742 2744 +2
Misses 311 311
☔ View full report in Codecov by Sentry. |
JET Benchmark ResultJudge resultBenchmark Report for /home/runner/work/JET.jl/JET.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/JET.jl/JET.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/JET.jl/JET.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
Architecture: x86_64
|
|
Even though it solves my problem from JuliaGraphs/Graphs.jl#249, I'm a bit skeptical about this. I thought @timholy also agreed that returning |
|
Currently, this behavior is only enabled in |
|
I agree that having it |
As requested at <#543 (comment)>.
As requested at <#543 (comment)>.
…gurable" This reverts commit 6f03664.
In the
analyze_from_definitionsmode, the analysis begins with the method signature. However, these signatures can often be abstract, leading to situations where, in the case ofx == 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 thatx == ycould returnmissing, and subsequently result in an error report, which, as discussed in #542, is usually an undesired false positive.analyze_from_definitionswas 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 ofmissingbeing returned should still be considered.A bit off-topic, but while making this change, I realized that the current design of
ReportPassmight not be functioning as effectively as hoped. Originally,ReportPasswas 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