-
Notifications
You must be signed in to change notification settings - Fork 5
add @testinferred
#11
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.
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
Conversation
src/testinferred.jl
Outdated
| It is useful to check for type stability. | ||
| This is similar to `Test.@inferred`, but instead of throwing an error, a `@test` is added. | ||
|
|
||
| Optionally, `AllowedType` relaxes the test, by making it pass when either the type of `f(x)` matches the inferred type modulo `AllowedType`, or when the return type is a subtype of `AllowedType`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to show an example here of the usage of AllowedType
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11 +/- ##
==========================================
+ Coverage 75.00% 77.51% +2.51%
==========================================
Files 4 4
Lines 248 338 +90
==========================================
+ Hits 186 262 +76
- Misses 62 76 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ok, I ended up completely redesigning it, such that more code is shared, and it also addresses #9 . Probably needs a bit of cleanup in assigning the documentation to the different macros, and maybe adding a few more tests, but I think the main functionality is working. But maybe I'll await some comments? |
lkdvos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the implementation in detail, but overall this looks like a nice improvement. I'm not sure how useful it is to have both a constinferred keyword to @testinferred, and a macro. I think I quite like the two macros instead, but this is obviously just subjective
|
It started with issue #9 that wanted the The two different macros remain available and for |
lkdvos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely see the use case for the broken as a keyword, since that can also dynamically be changed, it's mostly the constprop that I am less sure about. Ultimately, this does not hurt me though, so happy to get this merged and released as is!
|
Ok, I might just try to increase the coverage a bit further, so that the keyword approach is also tested and then have this merged. |
This adds the convenience
@testinferred, which behaves like@constinferredin the sense that it adds a test for inference, but doesn't try to capture constant propagation, since the way this is done in@constinferredis fundamentally incompatible with adding this to a non-toplevel expression in julia 1.12+:I tried to minimize the amount of depending on internal functionality, but some of this is a bit black magic to me. Fixes #10