Skip to content

Conversation

@timholy
Copy link
Member

@timholy timholy commented Sep 8, 2021

I found an example of a method declaration with an argument
typed as a::Vararg, and this triggered an error in showing method
candidates. It's always bad when there's an error triggered when handling
an error. This PR makes the code more robust against such problems.

It's worth noting that this type of method declaration
shouldn't be used: it is better as a.... The only time you should
explicitly use Vararg is when you are specifying both T and
N in Vararg{T,N}.

I found an example of a method declaration with an argument
typed as `a::Vararg`, and this triggered an error in showing method
candidates. It's always bad when there's an error triggered when handling
an error. This PR makes the code more robust against such problems.

It's worth noting that this type of method declaration
shouldn't be used:  it is better as `a...`. The only time you should
explicitly use `Vararg` is when you are specifying both `T` and
`N` in `Vararg{T,N}`.
timholy added a commit to JuliaArrays/OffsetArrays.jl that referenced this pull request Sep 8, 2021
Shorter, more readable, and doesn't break Julia.
What's not to like?

xref JuliaLang/julia#42161
@vtjnash
Copy link
Member

vtjnash commented Sep 8, 2021

@Keno Do we need to keep the undef property for .T of Tuple{Int, Vararg} vs. Tuple{Int, Vararg{Any}}`, or would it be better if we always normalized it during apply_type?

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Sep 8, 2021
timholy added a commit to JuliaArrays/OffsetArrays.jl that referenced this pull request Sep 8, 2021
Shorter, more readable, and doesn't break Julia.
What's not to like?

xref JuliaLang/julia#42161
@timholy timholy merged commit 867d80d into master Sep 8, 2021
@timholy timholy deleted the teh/method_candidates_va branch September 8, 2021 21:27
@Keno
Copy link
Member

Keno commented Sep 8, 2021

@Keno Do we need to keep the undef property for .T of Tuple{Int, Vararg} vs. Tuple{Int, Vararg{Any}}`, or would it be better if we always normalized it during apply_type?

I think it's probably fine to normalize inside Tuple{}, but outside of it, they do behave differently under apply_type, so the distinction needs to be kept.

@vtjnash
Copy link
Member

vtjnash commented Sep 8, 2021

Does it appear anywhere else?

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 9, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
I found an example of a method declaration with an argument
typed as `a::Vararg`, and this triggered an error in showing method
candidates. It's always bad when there's an error triggered when handling
an error. This PR makes the code more robust against such problems.

It's worth noting that this type of method declaration
shouldn't be used:  it is better as `a...`. The only time you should
explicitly use `Vararg` is when you are specifying both `T` and
`N` in `Vararg{T,N}`.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
I found an example of a method declaration with an argument
typed as `a::Vararg`, and this triggered an error in showing method
candidates. It's always bad when there's an error triggered when handling
an error. This PR makes the code more robust against such problems.

It's worth noting that this type of method declaration
shouldn't be used:  it is better as `a...`. The only time you should
explicitly use `Vararg` is when you are specifying both `T` and
`N` in `Vararg{T,N}`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants