-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add optional argument to @inferred that relaxes the case of Nothing and Missing #27516
Add optional argument to @inferred that relaxes the case of Nothing and Missing #27516
Conversation
a1fdab0
to
a3294e6
Compare
Thanks! I agree it's useful, but would a syntax like |
Yeah, I think extending
|
a3294e6
to
1263dd4
Compare
Putting the type argument first seems more legible and intuitive to me. |
If you need to explicitly give the expected return type, what's the advantage compared to a type assertion? |
You don't know what the inferred type minus the expected / allowed return type is. For example: function iterate(it, state)
some_condition && return nothing
new_value = ...
new_state = ...
new_value, new_state
end I know With the old behaviour of function produce_new_state_and_value(it, state)
new_value = ...
new_state = ...
new_value, new_state
end
function iterate(it, state)
some_condition && return nothing
produce_new_state_and_value(it, state)
end
@inferred produce_new_state_and_value(someiterator, somestate) but often these functions are just 3 lines long, and it wouldn't make sense to define a new function just to test inference. With this PR, you would just run @inferred Nothing iterate(someiterator, somestate) I think this test is very useful now that |
stdlib/Test/src/Test.jl
Outdated
@@ -21,7 +21,7 @@ export @test, @test_throws, @test_broken, @test_skip, | |||
|
|||
export @testset | |||
# Legacy approximate testing functions, yet to be included | |||
export @inferred | |||
export @inferred, @weakly_inferred |
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.
This should be removed since you're no longer defining it.
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.
Thanks!
Right, sorry, I had misinterpreted the proposal here. Carry on. |
Is this ready to be merged? Then I could use it in #27511 :) |
Yeah, looks good to me. Though perhaps we should rerun CI since it's been a couple days? |
047b715
to
dec2417
Compare
Circleci is killed during the build, and Travis probably the same? Other look good. |
The CI failures are doctest failures. |
dec2417
to
3379a94
Compare
Fixed the doctest issue, things look good. Not sure why circleci is failing. |
e36a2e4
to
2264cdb
Compare
Too bad this wasn't merged. Could you rebase it? |
2264cdb
to
01d8124
Compare
Sure |
01d8124
to
fa19651
Compare
fa19651
to
a50a8ca
Compare
Error has to do with the maximum PR?
|
Looks like problem with stack traces (probably not related)... :-/ |
Tests are green now @nalimilan |
Would be good with a news entry so people can more easily discover this :) |
Julia 1.0's cumsum doesn't work for iterators -> Collect first @inferred didn't support Allowed types until v1.2 - Added in JuliaLang/julia#27516 - Add VERSION check and only run type stability checks if >= v1.2 .≈ does seem to be defined in v1.0 (Weird) - Use separate Eq Checks if 1D or 2D (Don't need to map at all if 1D) - Replace broadcasting with map -> Seems to work
Brought up in #27511, where I want to test inference of an
iterate
call, yet@inferred
is not applicable because the return type isUnion{Nothing, T}
by design.The fix is to have
@weakly_inferred
, which does the same as@inferred
, but relaxes the cases ofnothing
andmissing
:If there's a better name for the macro, I'll be happy to change it.
Edit: final contribution in this PR is to add an optional type as argument to
@inferred
which is "white listed":