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 tmerge for (partially) const types with undef fields #43812

Merged
merged 3 commits into from
Jan 19, 2022
Merged

Conversation

martinholters
Copy link
Member

When tmerge is applied to Const/PartialStruct and a field is Const in one of the types and Union{} (i.e. undef) in the other, the resulting field type must not be Const (as the resursively called tmerge produces).

Fixes #43784.

When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) in the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).

Fixes #43784.
@martinholters martinholters added bugfix This change fixes an existing bug compiler:inference Type inference labels Jan 14, 2022
@martinholters
Copy link
Member Author

Side note: Looks like the bug was exposed by #43305 which made getfield_tfunc return Union{} for undefined fields instead of their declared type. So it's probably not necessary to backport this patch.

@martinholters
Copy link
Member Author

Ci failure on FreeBSD is unrelated Download failure ("Connection time-out while requesting https://httpbingo.julialang.org/base64/SnVsaWEgaXMgZ3JlYXQh").

Co-authored-by: Simeon Schaub <schaub@mit.edu>
@JeffBezanson
Copy link
Member

Could we instead modify to handle undefined fields better? It calls getfield directly, when it could call getfield_tfunc, and then it should just work.

@giordano
Copy link
Contributor

giordano commented Jan 14, 2022

I can confirm this fixes also #43561:

     Testing Running tests...
Test Summary: | Pass  Total  Time
Base          |    7      7  1.5s
Test Summary: | Pass  Total  Time
Utils         |    2      2  0.3s
Test Summary: | Pass  Total  Time
Promotion     |    6      6  3.4s
Test Summary: | Pass  Total  Time
Maths         |   18     18  1.3s
Test Summary: | Pass  Total  Time
Show          |    4      4  1.5s
     Testing PhysicalConstants tests passed 

julia> versioninfo()
Julia Version 1.8.0-DEV.1315
Commit 73d46513d1 (2022-01-14 11:20 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-13.0.0 (ORCJIT, haswell)

the fact that PhysicalConstants.jl hangs while precompiling (#43561) is due to the fact that Measurements.jl is broken (#43784)

@vtjnash
Copy link
Member

vtjnash commented Jan 14, 2022

Could we instead modify to handle undefined fields better? It calls getfield directly, when it could call getfield_tfunc, and then it should just work.

You cannot. This PR is currently exploiting that flaw to achieve convergence here. When that is improved to "just work", we will need to also add this case to issimpleenoughtype, to prohibit complexity from accumulating here.

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
@martinholters
Copy link
Member Author

CI failure is a timeout in the Distributed test which I'd think is unrelated. Good to go?

@aviatesk
Copy link
Member

yep, let's go.

@aviatesk aviatesk merged commit 5864e43 into master Jan 19, 2022
@aviatesk aviatesk deleted the mh/fix-43784 branch January 19, 2022 06:23
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…#43812)

When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) in the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).

Fixes JuliaLang#43784.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…#43812)

When `tmerge` is applied to `Const`/`PartialStruct` and a field is
`Const` in one of the types and `Union{}` (i.e. undef) in the other,
the resulting field type must not be `Const` (as the resursively called
`tmerge` produces).

Fixes JuliaLang#43784.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PartialStruct tmerge fails to converge
6 participants