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

Improve nothrow analysis of :new with missing sparam #46754

Merged
merged 1 commit into from
Sep 15, 2022
Merged

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 14, 2022

Similar to #46693, but for :new, rather than getfield. Unfortunately, this is somewhat limited as we don't really have the ability to encode type equality constraints in the lattice. In particular, it would be nice to analyze:

struct Foo{T}
    x::T
    Foo(x::T) where {T} = new{T}(x)
end

and be able to prove this nothrow. If it's really
important, we could probably pattern match it, but for the moment, this is not easy to do. Nevertheless, we can do something about the similar, but simpler pattern

struct Foo{T}
    x
    Foo(x::T) where {T} = new{T}(x)
end

which is what this PR does.

Similar to #46693, but for :new, rather than getfield.
Unfortunately, this is somewhat limited as we don't really
have the ability to encode type equality constraints in the
lattice. In particular, it would be nice to analyze:

```
struct Foo{T}
    x::T
    Foo(x::T) where {T} = new{T}(x)
end
```

and be able to prove this nothrow. If it's really
important, we could probably pattern match it, but
for the moment, this is not easy to do. Nevertheless,
we can do something about the similar, but simpler pattern

```
struct Foo{T}
    x
    Foo(x::T) where {T} = new{T}(x)
end
```

which is what this PR does.
typ, isexact = instanceof_tfunc(atyp)
if !isexact
atyp = unwrap_unionall(widenconst(atyp))
if isType(atyp) && isTypeDataType(atyp.parameters[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought at some point we had to deal with this check being insufficient and incomplete due to the existence of free-type-vars, for example if the user called:

julia> Val(Val.body);

which I thought used to be an issue for convert and Fix1 calls (particularly those implied by fieldtypes)

But perhaps we don't handle that case anyways, since that call segfaults, and this call poisons the dispatch cache permanently:

julia> Base.Fix1(Val.body, 1)
ERROR: UndefVarError: F not defined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see test/core.jl for TypeError("new", ...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Types with missing sparams are not valid ::Types, because they error in subtyping. They can only dispatch as ::DataType. There's a few places in the code, where we aren't careful about it, but we just need to clean those up. I don't think :new with a type with a free tvar is legal either, but absent other bugs, I don't think this code allows that.

@Keno Keno merged commit 94c3a15 into master Sep 15, 2022
@Keno Keno deleted the kf/newsparameffects branch September 15, 2022 05:24
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.

2 participants