Skip to content

Fix tmerge for (partially) const types with undef fields#43812

Merged
aviatesk merged 3 commits intomasterfrom
mh/fix-43784
Jan 19, 2022
Merged

Fix tmerge for (partially) const types with undef fields#43812
aviatesk merged 3 commits intomasterfrom
mh/fix-43784

Conversation

@martinholters
Copy link
Copy Markdown
Member

@martinholters martinholters commented Jan 14, 2022

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
Fix tmerge for (partially) const types with undef fields
9116078 
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
Copy Markdown
Member Author

martinholters commented Jan 14, 2022

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
Copy Markdown
Member Author

martinholters commented Jan 14, 2022

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

simeonschaub
simeonschaub reviewed Jan 14, 2022
View reviewed changes
@martinholters @simeonschaub
Use === in a comparison to Union{}
73d4651 
Co-authored-by: Simeon Schaub <schaub@mit.edu>
@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson 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.

vtjnash
vtjnash reviewed Jan 14, 2022
View reviewed changes
@giordano
Copy link
Copy Markdown
Member

giordano commented Jan 14, 2022 •
edited
Loading

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
Copy Markdown
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.

aviatesk
aviatesk reviewed Jan 15, 2022
View reviewed changes
@martinholters @aviatesk
Fix test for #43784
c7ef143 
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
@martinholters
Copy link
Copy Markdown
Member Author

martinholters commented Jan 19, 2022

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

@aviatesk
Copy link
Copy Markdown
Member

aviatesk commented Jan 19, 2022

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
@Keno Keno mentioned this pull request Jan 19, 2022
Closed
@N5N3 N5N3 mentioned this pull request Jan 19, 2022
Closed
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@martinholters @LilithHafner
Fix tmerge for (partially) const types with undef fields (JuliaLang…
8082a4b 
…#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
@martinholters @LilithHafner
Fix tmerge for (partially) const types with undef fields (JuliaLang…
efac294 
…#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

Reviewers

@vtjnash vtjnash vtjnash left review comments

@simeonschaub simeonschaub simeonschaub left review comments

@aviatesk aviatesk aviatesk approved these changes

Assignees

No one assigned

Labels

bugfix This change fixes an existing bug compiler:inference Type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

PartialStruct tmerge fails to converge

6 participants

@martinholters @JeffBezanson @giordano @vtjnash @aviatesk @simeonschaub