Skip to content
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

Unoptimal inference when function has error in a implicit return #54029

Closed
gbaraldi opened this issue Apr 10, 2024 · 3 comments · Fixed by #54086
Closed

Unoptimal inference when function has error in a implicit return #54029

gbaraldi opened this issue Apr 10, 2024 · 3 comments · Fixed by #54086

Comments

@gbaraldi
Copy link
Member

While investigating some type instabilities in Pkg code specifically

	code_typed(Pkg.Artifacts.:(var"#ensure_artifact_installed#21"), (Base.BinaryPlatforms.Platform, Bool, Bool, IO, typeof(Pkg.Artifacts.ensure_artifact_installed), String, Dict, String), optimize=true)

the #21 might not be correct but it's the kwcall done in code_typed(Core.kwcall, (@NamedTuple{platform::Base.BinaryPlatforms.Platform}, typeof(Pkg.Artifacts.ensure_artifact_installed), String, Dict{String, Any}, String))
this code has a unstable return type because inference isn't able to succeed in realising that a call to error always throws

│     %486 = Base.not_int(%485)::Bool
└────        goto #107 if not %486
106 ─        goto #97
107 ┄ %489 = φ (#105 => %453, #96 => %405)::Any
│     %490 = Pkg.Artifacts.error(%489)::Any
└────        return %490
108 ─ %492 = π (%2, Base.SHA1)
│     %493 = Pkg.Artifacts.artifact_path::typeof(Artifacts.artifact_path)
│     %494 = invoke Artifacts.:(var"#artifact_path#7")(true::Bool, %493::typeof(Artifacts.artifact_path), %492::Base.SHA1)::String
└────        return %494
@KristofferC
Copy link
Member

Fixed by #49260 I guess

@aviatesk
Copy link
Member

It turns out that this is because Pkg.jl set @max_methods 2 globally: https://github.com/JuliaLang/Pkg.jl/blob/00aa38cb3a85e200c4bd19efef3a15e32e192a8f/src/Pkg.jl#L5-L7

I have a potential solution in my mind, but it looks like it won't require merging #49260.

aviatesk added a commit that referenced this issue Apr 15, 2024
It is important to always be able to infer the return type of `error`
as `Union{}`, but there's a hitch when a package globally sets
`@max_methods 1` and it causes inference for `error(::Any)` to fail
(#54029).

To address this, this commit sets `@max_methods 2 function error end`
at the definition of `error`. This overrides any global
`@max_methods 1` settings on package side, guaranteeing that return type
inference on `error` is successful always.
@aviatesk
Copy link
Member

See #54086.

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 a pull request may close this issue.

3 participants