Refine type for opaque (remain user_type) #276
Conversation
|
Not sure if I can create a failing test case. I expect it to fail if the |
Codecov Report
@@ Coverage Diff @@
## master #276 +/- ##
==========================================
+ Coverage 87.64% 87.64% +<.01%
==========================================
Files 18 18
Lines 2630 2631 +1
==========================================
+ Hits 2305 2306 +1
Misses 325 325
Continue to review full report at Codecov.
|
You could create a failing test case by trying to use |
| -opaque my_opaque() :: integer(). | ||
|
|
||
| -spec internal(my_opaque(), integer() | undefined) -> integer(). | ||
| internal(_, undefined) -> 0; |
There was a problem hiding this comment.
I'd expect a test that the type is still visible from inside the module, i.e. that the first parameter is used in this function.
|
Sorry I guess I was in a rush a bit 😅 . Will try to include these test cases tomorrow. |
| refine_ty({user_type, Anno, Name, Args}, {user_type, Anno, Name, Args}, _TEnv) -> | ||
| % After being normalized, it's because it's defined as opaque. | ||
| % If it has the same annotation, name and args, it's the same. | ||
| type(none); |
There was a problem hiding this comment.
There is a clause like this above:
refine_ty(?type(T, A), ?type(T, A), _) ->
type(none);Perhaps both these could be replaced by simply one clause like this:
refine_ty(Same, Same, _TEnv) ->
%% If it's the same, it's the same
type(none);What do you think? Future improvement?
There was a problem hiding this comment.
The only thing is this would also catch a few other cases I haven't put thoughts into. From looking at gradualizer_type.erl, The other stuff that would pass:
ann_type: Are these supposed to be removed at that point? I think I saw a PR go by mentioning this but haven't looked into it enough.var: Honestly have no clue, I have barely touched type variables at this point.remote_type: Pretty sure normalize catches this and either expand it in atype, or auser_typeif it's opaque.user_type: D'uh. After normalization, the only way it's still auser_typeis an opaque type from another module. Seems fine and would remove the case I wrote.
I created my other PR #277 but not sure I wanna include them. I merged this because I'm using our existant codebase to test my record refinement branch and I needed this change 😄
There was a problem hiding this comment.
OK!
Anything minus itself would be none() I guess, even a type variable. (Then, perhaps we also may want to add other cases such as SomeType \ TypeVar and vice versa and add constraints.)
|
Very good! I think it's good that you merged, because it was already complete and working. More tests, refactoring, etc. can always be done later. |
Allow type refinement in the presence of matching opaque types.
Allow opaque types to further refinement.