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 NTuple show and cleanup code for Vararg and NTuple type parameter construction #51244

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Sep 8, 2023

Refs #51228

@vtjnash Why did you mention that the printing of NTuple needs to be adjusted? Your example still works fine:

julia> Tuple{1,1,1,1}
NTuple{4, 1}

julia> NTuple{4, 1}
NTuple{4, 1}

... even though I guess the same validation could happen for NTuple, especially because it's just a Vararg:

julia> NTuple
Tuple{Vararg{T, N}} where {N, T}

julia> Vararg{4, 1}
ERROR: TypeError: in Vararg, in type, expected Type, got a value of type Int64

EDIT: AFAICT, it doesn't seem as easy to reject NTuple{1,2}, because instantiation happens in two steps: First, inst_tuple_w_ instantiates this to a fixed-length tuple, but the invalid T=2 doesn't seem known there (there's only T=T and N=1 in the type environment). Then, inst_datatype_inner instantiates the Tuple, but we can't reject T=2 there as Tuple{2} is valid.

@maleadt maleadt added domain:types and dispatch Types, subtyping and method dispatch kind:bugfix This change fixes an existing bug labels Sep 8, 2023
@N5N3
Copy link
Member

N5N3 commented Sep 8, 2023

Is it possible to only apply this fix to method signature?
Our StaticArray ecosystem uses Tuple with Int parameters to store Size information, so I think it would be good to avoid inconsistency.

@maleadt
Copy link
Member Author

maleadt commented Sep 8, 2023

Our StaticArray ecosystem uses Tuple with Int parameters to store Size information, so I think it would be good to avoid inconsistency.

The PR currently only addresses Vararg. I tried looking into restricting NTuple but not Tuple, but that didn't work out (see the edit above), so nothing should change wrt. Tuple behavior for now.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 8, 2023

Current status of the testsuite with this added restriction looks like:

This can be fixed in Test:

Test Failed at /home/vtjnash/julia1/test/ambiguous.jl:360
  Expression: need_to_handle_undef_sparam == Set()
   Evaluated: Set(Method[_extrema_rf(x::Tuple{Vararg{T, 2}}, y::Tuple{Vararg{T, 2}}) where T<:Union{Float16, Float32, Float64} @ Base reduce.jl:867]) == Set{Any}()

This is a failure of inference, due to failure to evaluate nfields(NTuple{4}) === 4

Test Failed at /home/vtjnash/julia1/test/tuple.jl:761
  Expression: only(Base.return_types(g42457, (NTuple{3, Int}, NTuple{4}))) === Float64
   Evaluated: Union{Float64, Int64} === Float64
julia> code_warntype(Base.length, (NTuple{4},))
MethodInstance for length(::Tuple{Vararg{T, 4}} where T)
  from length(t::Tuple) @ Base tuple.jl:26
Arguments
  #self#::Core.Const(length)
  t::TUPLE{VARARG{T, 4}} WHERE T
Body::Int64
    @ tuple.jl:26 within `length`
1 ─      nothing
│   %2 = Base.nfields(t)::Int64
└──      return %2

These ones we meant to break here:

Error During Test at /home/vtjnash/julia1/test/core.jl:7845
  Test threw exception
  Expression: NTuple{3, 2} == Tuple{2, 2, 2}
  TypeError: in Vararg, in type, expected Type, got a value of type Int64
  Stacktrace:
   [1] macro expansion
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:676 [inlined]
   [2] top-level scope
     @ ~/julia1/test/core.jl:515
Error During Test at /home/vtjnash/julia1/test/show.jl:1376                                                                                                                                             
  Test threw exception                                                                                                                                                                                  
  Expression: repr(NTuple{4, :A}) == "NTuple{4, :A}"                                                                                                                                                    
  TypeError: in Vararg, in type, expected Type, got a value of type Symbol                                                                                                                              
  Stacktrace:                                                                                                                                                                                           
   [1] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:676 [inlined]                                                                                                                             
   [2] macro expansion                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1376 [inlined]                                                                                                                                                             
   [3] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:1599 [inlined]                                                                                                                            
   [4] top-level scope                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1376                                                                                                                                                                       
Error During Test at /home/vtjnash/julia1/test/show.jl:1377                                                                                                                                             
  Test threw exception                                                                                                                                                                                  
  Expression: repr(NTuple{3, :A}) == "Tuple{:A, :A, :A}"                                                                                                                                                
  TypeError: in Vararg, in type, expected Type, got a value of type Symbol                                                                                                                              
  Stacktrace:                                                                                                                                                                                           
   [1] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:676 [inlined]                                                                                                                             
   [2] macro expansion                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1377 [inlined]                                                                                                                                                             
   [3] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:1599 [inlined]                                                                                                                            
   [4] top-level scope                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1376                                                                                                                                                                       
Error During Test at /home/vtjnash/julia1/test/show.jl:1378                                                                                                                                             
  Test threw exception                                                                                                                                                                                  
  Expression: repr(NTuple{2, :A}) == "Tuple{:A, :A}"                                                                                                                                                    
  TypeError: in Vararg, in type, expected Type, got a value of type Symbol                                                                                                                              
  Stacktrace:                                                                                                                                                                                           
   [1] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:676 [inlined]                                                                                                                             
   [2] macro expansion                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1378 [inlined]                                                                                                                                                             
   [3] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:1599 [inlined]                                                                                                                               [4] top-level scope                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1376                                                                                                                                                                       
Error During Test at /home/vtjnash/julia1/test/show.jl:1379                                                                                                                                             
  Test threw exception                                                                                                                                                                                  
  Expression: repr(NTuple{1, :A}) == "Tuple{:A}"                                                                                                                                                        
  TypeError: in Vararg, in type, expected Type, got a value of type Symbol                                                                                                                              
  Stacktrace:                                                                                                                                                                                           
   [1] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:676 [inlined]                                                                                                                             
   [2] macro expansion                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1379 [inlined]                                                                                                                                                             
   [3] macro expansion                                                                                                                                                                                  
     @ ~/julia1/usr/share/julia/stdlib/v1.11/Test/src/Test.jl:1599 [inlined]                                                                                                                            
   [4] top-level scope                                                                                                                                                                                  
     @ ~/julia1/test/show.jl:1376                                                                                                                                                                       

@maleadt
Copy link
Member Author

maleadt commented Sep 11, 2023

These ones we meant to break here:

I fixed those.

@maleadt
Copy link
Member Author

maleadt commented Sep 11, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 11, 2023

Ah, right, this approach will probably never work, since StaticArrays assumes this is valid to use to represent array dimension tuples. For example, an SMatrix{2, 2, Int64} is actually an SArray{Tuple{2, 2}, Int64, 2, 4}

@maleadt
Copy link
Member Author

maleadt commented Sep 12, 2023

StaticArrays assumes this is valid to use to represent array dimension tuples. For example, an SMatrix{2, 2, Int64} is actually an SArray{Tuple{2, 2}, Int64, 2, 4}

We're only disallowing NTuple{2,2} though; Tuple{2,2} still works. However, it looks like packages are using the former, e.g. RegionTrees.jl has NTuple{N, 2} where N, which accounts for most of the failures in the PkgEval run.

I guess it doesn't make sense to disallow NTuple{2,2} if we're still allowing Tuple{2,2}... I'll revert that part of the changes and see if just the Vararg change is breaking.

@maleadt
Copy link
Member Author

maleadt commented Sep 12, 2023

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@maleadt
Copy link
Member Author

maleadt commented Sep 13, 2023

OK, that didn't work because NTuple is of course defined as Tuple{Vararg{T, N}}, so the new restriction on Vararg breaks NTuple{2,2}.

Working around this now by special-casing the Vararg handling when contained in a NTuple (by temporarily swapping in a valid type for T, which is a hack, but we cannot simply disable check because N still needs to be checked). This makes NTuple{2,2} work, however, Tuple{Vararg{2,2}} doesn't because the types are applied separately. So all pretty unsatisfying.

@maleadt
Copy link
Member Author

maleadt commented Sep 13, 2023

@nanosoldier runtests(["RegionTrees", "ChaoticEncryption", "MIRTjim", "ClusterDepth", "MRIReco", "Roentgen", "Test", "ImageHashes", "MRIFiles", "LASindex", "SMLMFrameConnection", "MiseEnPage", "CellSegmentation", "SSIMLoss", "WGPUgfx", "Dynesty", "Eikonal", "ImageUtils", "StructuredLight", "Images", "MRICoilSensitivities", "SMLMSim", "FatDatasets", "AvailablePotentialEnergyFramework", "FinEtoolsVoxelMesher", "LineIntegralConvolution", "NeuroAnalysis", "Mellan", "ImageSegmentation", "OVERT", "SMLMMetrics", "MRISimulation", "HiQGA", "QuantumCumulants", "Fable", "Colocalization", "ImageProjectiveGeometry", "ImagineFormat", "EditorsRepo", "HmtArchive", "BundlerIO", "PyBraket", "GIFImages", "MaterialReconstruction", "AztecDiamonds", "PlutoStaticHTML", "PerceptualColourMaps", "RemoteSensingToolbox", "ImageFeatures", "ColorSchemeTools", "CorrelationTrackers", "SMLMData", "CitableImage", "FaceDetection", "SyntacticModels", "MPITestImages", "CorrelationFunctions", "RoentgenPlots", "BM3DDenoise", "DiffEqCallbacks", "MixtureDensityNetworks", "RetentionParameterEstimator"])

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@maleadt
Copy link
Member Author

maleadt commented Sep 13, 2023

@nanosoldier runtests(["RegionTrees", "ChaoticEncryption", "MIRTjim", "ClusterDepth", "MRIReco", "Roentgen", "Test", "ImageHashes", "MRIFiles", "LASindex", "SMLMFrameConnection", "MiseEnPage", "CellSegmentation", "SSIMLoss", "WGPUgfx", "Dynesty", "Eikonal", "ImageUtils", "StructuredLight", "Images", "MRICoilSensitivities", "SMLMSim", "FatDatasets", "AvailablePotentialEnergyFramework", "FinEtoolsVoxelMesher", "LineIntegralConvolution", "NeuroAnalysis", "Mellan", "ImageSegmentation", "OVERT", "SMLMMetrics", "MRISimulation", "HiQGA", "QuantumCumulants", "Fable", "Colocalization", "ImageProjectiveGeometry", "ImagineFormat", "EditorsRepo", "HmtArchive", "BundlerIO", "PyBraket", "GIFImages", "MaterialReconstruction", "AztecDiamonds", "PlutoStaticHTML", "PerceptualColourMaps", "RemoteSensingToolbox", "ImageFeatures", "ColorSchemeTools", "CorrelationTrackers", "SMLMData", "CitableImage", "FaceDetection", "SyntacticModels", "MPITestImages", "CorrelationFunctions", "RoentgenPlots", "BM3DDenoise", "DiffEqCallbacks", "MixtureDensityNetworks", "RetentionParameterEstimator"])

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@vtjnash vtjnash force-pushed the jn/51228 branch 2 times, most recently from f793245 to 4e66163 Compare September 13, 2023 16:38
@vtjnash vtjnash marked this pull request as ready for review September 13, 2023 16:39
@vtjnash vtjnash changed the title Validate Vararg type parameter on construction improve NTuple show and cleanup code for Vararg and NTuple type parameter construction Sep 13, 2023
@vtjnash vtjnash removed the kind:bugfix This change fixes an existing bug label Sep 13, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 13, 2023

I have moved the minimal bugfix to #51300 instead, and kept this PR as just the code cleanups. It also now implements a better show method still as well, which handles fixed length Vararg and bound NTuple more naturally.

Various simplifications and improvements from investigating #51228.
Improves the logic for showing of NTuple to handle constant lengths.
Improves the logic for showing NTuple of bound length (e.g. NTuple
itself). Also makes a choice to avoid showing non-types as NTuple, but
instead try to write them out, to make it more visually obvious when the
parameters have been swapped.
@JeffBezanson JeffBezanson merged commit 0287a00 into master Sep 25, 2023
6 checks passed
@JeffBezanson JeffBezanson deleted the jn/51228 branch September 25, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants