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

Fix anymutable errors with self-referencing types #72

Merged
merged 4 commits into from
Feb 7, 2024

Conversation

oschulz
Copy link
Contributor

@oschulz oschulz commented Feb 6, 2024

I currently have trouble using fmap with data structures that contains Measurements.Measurement objects. The reason is that Measurement (which subtypes Real) contains a field Derivatives:

struct Derivatives{T} <: AbstractDict{Tuple{T,T,UInt64},T}
    parent::Derivatives{T}
    ...
end

Due to the nature of the type, this parent field may be undefined - quite normal for such datatypes.(Measurements.Derivatives is a variation of Base.ImmutableDict, apparently).

This causes Functors.anymutable to fail, due to

@generated function anymutable(x::T) where {T}
  ...
  subs =  [:(anymutable(getfield(x, $f))) for f in QuoteNode.(fieldnames(T))]
  ...
end

While I have this problem with Measurement specifically, I think it makes sense to fix it here, instead of proposing a Functors extension to Measurements. Also, as expected (see above) the same problem occurs with Base.ImmutableDict:

julia> fmap(identity, Base.ImmutableDict(:b => 42))
ERROR: UndefRefError: access to undefined reference

A simple fix would be checking with isdefined (if the field is not defined, it obviously doesn't point to mutable content):

@generated function anymutable(x::T) where {T}
  ...
  subs =  [:(isdefined(x, $f) && anymutable(getfield(x, $f))) for f in QuoteNode.(fieldnames(T))]
  ...
end

I worry that this may reduce the type stability of anymutable though. So in this PR I'm proposing

@generated function anymutable(x::T) where {T}
  ...
  fns = QuoteNode.(filter(n -> fieldtype(T, n) != T, fieldnames(T)))
  subs =  [:(anymutable(getfield(x, $f))) for f in fns]
  ...
end

In the end, mutability is a type property, I'd say: if T itself is not directly mutable, and all of it's fields except references to T are immutable, then I think it's safe to consider x immutable as a whole.

@oschulz oschulz changed the title Make anymutable work with self-referencing types Fix anymutable errors with self-referencing types Feb 6, 2024
@oschulz
Copy link
Contributor Author

oschulz commented Feb 7, 2024

Also fixes the broken docs generation (was not Documenter v1 compatible, but Documenter version bound was missing in "docs/Project.toml"). I updated everything to Documenter v1 now.

@oschulz
Copy link
Contributor Author

oschulz commented Feb 7, 2024

@ToucheSir, if you have time, could you take a look at this?

Copy link
Member

@ToucheSir ToucheSir left a comment

Choose a reason for hiding this comment

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

I didn't quite understand what this was doing yesterday, but with a fresh set of eyes it makes sense now :)

@ToucheSir ToucheSir merged commit 942126f into FluxML:master Feb 7, 2024
6 checks passed
@oschulz
Copy link
Contributor Author

oschulz commented Feb 8, 2024

Whoops, sorry @ToucheSir , I had overlooked the fact that the version number had already been increased, so JuliaRegistries/General#100422 is currently stalled.

Could you call Registrator on commit 19788ea so that we can get v0.4.6 registered first?

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