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

getproperty(::Type,f::Symbol): type assert when f === :parameters #57630

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Mar 4, 2025

It is common to access the parameters field of a DataType using getproperties. However, when the first argument is not inferred as DataType, the return type infers as Any, instead of as Core.SimpleVector. I believe this causes much type instability and invalidation.

The proper fix would probably be to introduce a getter function, something like _get_datatype_parameters(x::DataType) = x.parameters, then use the getter instead of directly using getproperties. For Compiler, we may have to introduce a getter anyway, as Compiler.getproperty === Core.getfield, but outside of the compiler this seems like a good enough hacky fix.

It is common to access the `parameters` field of a `DataType` using
`getproperties`. However, when the first argument is not inferred as
`DataType`, the return type infers as `Any`, instead of as
`Core.SimpleVector`. I believe this causes much type instability and
invalidation.

The proper fix would probably be to introduce a getter function,
something like `_get_datatype_parameters(x::DataType) = x.parameters`,
then use the getter instead of directly using `getproperties`. For
`Compiler`, we may have to introduce a getter anyway, as
`Compiler.getproperty === Core.getfield`, but outside of the compiler
this seems like a good enough hacky fix.
@nsajko nsajko added types and dispatch Types, subtyping and method dispatch invalidations labels Mar 4, 2025
@nsajko
Copy link
Contributor Author

nsajko commented Mar 4, 2025

For Compiler, we may have to introduce a getter anyway

This is meant to be a follow up PR.

@Keno
Copy link
Member

Keno commented Mar 4, 2025

Can we instead improve the precision of the compiler model? I don't really like adding these kinds of special cases

@nsajko
Copy link
Contributor Author

nsajko commented Mar 4, 2025

Out of curiosity, are you saying the compiler should exploit the fact that the subtypes of Type are fixed (only typeof(Union{}), Union, UnionAll and DataType, where the latter is the only subtype with a parameters field)? Or are you proposing something more general?

@nsajko
Copy link
Contributor Author

nsajko commented Mar 4, 2025

I believe this causes much type instability and invalidation.

The PR only gets rid of 1% of the invalidations that happen when my package is loaded, so I misjudged.

@nsajko
Copy link
Contributor Author

nsajko commented Mar 4, 2025

I think I'll rather add DataType/Core.SimpleVector type asserts wherever necessary.

@nsajko nsajko closed this Mar 4, 2025
@nsajko nsajko deleted the getproperty_Type_parameters_field_type_assert branch March 4, 2025 01:16
@vtjnash
Copy link
Member

vtjnash commented Mar 4, 2025

If inference didn't work, it seems probably likely the program is relying on implementation details that may not be stable. Typically applications should not access parameters field or rely on it (unless they are doing something to implement those type system details themselves)

@nsajko
Copy link
Contributor Author

nsajko commented Mar 4, 2025

The parameters field is also accessed using getproperty from Base, not just from applications. See PR #57631.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalidations types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants