use more precise structural inspection of types for ambiguity exclusions #327
Conversation
Should be slightly more robust, since subtyping already implements these queries with much more finesse than is going to be true of a re-implementation of the query here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #327 +/- ##
==========================================
+ Coverage 86.93% 87.35% +0.41%
==========================================
Files 11 11
Lines 513 506 -7
==========================================
- Hits 446 442 -4
+ Misses 67 64 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Do you still care about v1.2- here? Apparently it needs some fix to avoid a julia bug, but I don't know if that is actually helpful to figure out? |
|
Thanks for the PR @vtjnash, from a first glance, this looks like a great simplification. I'll give this a more detailed review once the minimum supported julia version has been increased (to be done for the next minor release). |
|
I can also fix the support for those versions, I just wanted to check first if this PR and that support was considered useful |
| if num_ambiguities_ != num_ambiguities | ||
| @show exclude | ||
| println(strout) | ||
| println(strerr) | ||
| end |
There was a problem hiding this comment.
How is this related to the rest of the changes in here?
There was a problem hiding this comment.
Just debugging for the relevant tests if they change reporting
test/test_ambiguities.jl
Outdated
| @@ -44,7 +49,7 @@ include("preamble.jl") | |||
| check_testcase([PkgWithAmbiguities.ConcreteType], total - num_ambs_ConcreteType) | |||
|
|
|||
| # exclude abstract supertype without callables and constructors | |||
| check_testcase([PkgWithAmbiguities.AbstractType], total) | |||
| check_testcase([PkgWithAmbiguities.AbstractType], total - num_ambs_SingletonType - num_ambs_ConcreteType) | |||
There was a problem hiding this comment.
This seems like an unwanted behavioral change.
If I exclude AbstractType, according to the docstring, this excludes ambiguities of the constructor AbstractType and the functor (::AbstractType).
However, after this change, it also ignores ambiguities of the constructor SingletonType (which IMO is a completely different function) and the functor (::SingletonType) (which shares the methodtable with (::AbstractType) for each SingletonType object created).
The former change seems clearly unwanted to me, for the latter one I am not sure what to do best here. If possible, I think I would like to keep the current behavior.
Note, however, that with the current behavior, excluding ConcreteParameterizedType also excludes all constructors ConcreteParameterizedType{T} for any T.
Since we already have a breaking change in master, it would be fine to slightly change the semantics of exclusion here, but that would need some good explanation of the new semantics in the docstring of test_ambiguities and in the changelog.
There was a problem hiding this comment.
Okay, I've switched it back to doing only structural inspection instead of the subtyping queries and just simplified them to stay in the type domain where the comparisons can be made more reliable. It is less powerful (since it cannot realize that SingletonType is an implementation of AbstractType) but that keeps the existing named-based exclusion behavior.
vtjnash
changed the title
… types Even though that means there are cases that will be impossible to exclude, the author of Aqua confirmed that is intentional.
Should be slightly more robust, since subtyping already implements these queries with much more finesse than is going to be true of a re-implementation of the query here.