search

julia-vscode / SymbolServer.jl

Other
22 stars 31 forks source link

fix on Julia nightly #194

Closed simeonschaub closed 3 years ago

simeonschaub commented 3 years ago

see https://github.com/JuliaLang/julia/pull/38835 and https://github.com/JuliaLang/julia/pull/38136

simeonschaub commented 3 years ago

Hmm, ok, I am now not quite sure how FakeTypeName is supposed to work. Is it expected that something like Array{Int} just drops the parameters for the DataType Array{Int,TypeName(:N)} completely? It seems to be intentional because FakeTypeName is called with justname=true.

simeonschaub commented 3 years ago

Bump. What should be done about the tests here?

musm commented 3 years ago

bump, would love to be able to use this on 1.7

ZacLN commented 3 years ago

Hmm, ok, I am now not quite sure how FakeTypeName is supposed to work. Is it expected that something like Array{Int} just drops the parameters for the DataType Array{Int,TypeName(:N)} completely? It seems to be intentional because FakeTypeName is called with justname=true.

Yes, in it's current state this isn't intended to fully describe the types. Parameters are dropped except where at the first level of a DataType. For example given abstract type T{S} end we would have a full representaiton of T{Bool} but not of T{T{Bool}} and so FakeTypeName(T{T{SomeType}}) == FakeTypeName(T{T{SomeOtherType}}) is expected. This is to prevent storing 'massive' datatypes - there are some huge ones in LinearAlgebra. Given this, I don't think the tests are quite appropriate (i.e. they're asking too much) and happy for them to be dropped.

I've not had a look at the new approach to Varargs and a different heuristic may well be appropriate.

simeonschaub commented 3 years ago

I still kept most of the tests, but excluded some in earlier Julia versions. I hope they are not too strict now, but my thought was that they can always be loosened if they become a problem. This also now passes on justname=true for all type parameters of Vararg to be consistent with the previous behavior.

This is to prevent storing 'massive' datatypes - there are some huge ones in LinearAlgebra.

Do you mean things like StridedArray? Maybe we could piggyback off the type alias printing added to Base in https://github.com/JuliaLang/julia/pull/36107 here, so we only store the alias if there is a matching one.

ZacLN commented 3 years ago

Thanks, looks good. Yes exactly that, I'll have a look at the PR (thanks for the ref).

simeonschaub commented 3 years ago

Good to merge?

musm commented 3 years ago

Would also be great to have a tag with the fix.

ZacLN commented 3 years ago

Yes, staticlint will need small adjustments.