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

at-kwdef support for parametric types and subtypes #29316

Merged
merged 4 commits into from Oct 5, 2018

Conversation

5 participants
@simonbyrne
Copy link
Contributor

commented Sep 21, 2018

Fixes #29307.

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

I have no idea what is happening, but I get this locally:

Generating precompile statements...┌ Error: Failed to precompile precompile(Tuple{typeof(Base.show), Base.IOContext{Base.TTY}, Type{Vararg{Any, N} where N}})
└ @ Main.anonymous /Users/simon/src/julia/contrib/generate_precompile.jl:169
ERROR: LoadError: LoadError: TypeError: in Type, in parameter, expected Type, got Type{Vararg{Any,N} where N}

any ideas?

@KristofferC

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Seems like another manifestation of #28808.

There is already one workaround at

occursin("\"YYYY-mm-dd\\THH:MM:SS\"", statement) && continue
, perhaps just add this one as well?

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Adding this makes it compile:
statement == "precompile(Tuple{typeof(Base.show), Base.IOContext{Base.TTY}, Type{Vararg{Any, N} where N}})" && continue

I was not aware that @kwdef strives for "feature completeness". But this PR nearly puts it there, so whilst you're at it, maybe also add:

# allow inner constructors
# (assume one will be the full positional one)
Base.@kwdef struct A
    a = 1
    A(a) = (@assert a>1; new(a))
end
# -> ERROR: syntax: "A(a)" is not a valid function argument name

# Closures
const b = 7
Base.@kwdef struct B
    a::Int=b
end
B() # ERROR: UndefVarError: b not defined

Also, here some tests, adapted from Parameters.jl, which might be good to add: https://gist.github.com/mauro3/6a700f5b6fe09b4d0c130c354a8178ac. (although, I did not check which ones are redundant with the already existing ones).

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Thanks for the fix and the examples. I'm not sure what you mean by "feature completeness", but if it exists, it might as well be correct.

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Sorry, with "feature complete" I meant what the equivalent macro @with_kw in Parameters.jl supports. Looks like I can drop that part of Parameters.jl, thanks!

How about adding to the docs something like: "If inner constructors are defined, then @kwdef assumes that one is the positional constructor of all fields."

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Makes sense: what else is missing compared with @with_kw?

How about:

Inner constructors can still be defined, but should accept arguments in the same form as the default inner constructor (i.e. one positional argument per field) in order to function correctly with the keyword outer constructor.

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Four bits which are "missing" are:

  • allow @asserts in the type body which then get included in the positional inner constructor.
  • different show method.
  • type-specific unpack macros
  • automatic documentation of the type-fields with their defaults

I don't think the first three are suitable for Base. The last would be nice though, but maybe for another PR.

Inner constructors can still be defined, but one needs accept arguments in the same form as the default inner constructor (i.e. one positional argument per field) in order to function correctly with the keyword outer constructor.

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Wait, there is another useful one:

i1 =  SomeType(a=77, b=0,...)
i2 = SomeType(i1, b=9) # takes all of i1's values except the ones defined with the kw

x-ref: #5333

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

I can see that last one would be useful, but there is the question of what to do if the struct takes a single field. e.g.

struct SomeType
   a
end

then what should

SomeType(SomeType(a=1))

give?

@simonbyrne simonbyrne force-pushed the sb/kwdef branch from 4891396 to 28d8f98 Sep 24, 2018

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2018

Just SomeType(1), no?

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

How would you construct the nested type?

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

It's a pity we can't overload splatting, then we could use the syntax

SomeType(SomeType(a=1)...; a=2)

At the moment that would require writing an iterator over the object.

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

How would you construct the nested type?

I see the problem. But you already figured out the solution:

It's a pity we can't overload splatting

but you should probably implement that in a separate PR. ;-)

No, more seriously, probably leave that feature for now. Alternatively, just forget about the nested type in the one-field case.

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

Okay, I think this is ready to merge.

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2018

LGTM except the doc-string update:

Inner constructors can still be defined, but one needs to accept arguments in the same form as the default inner constructor (i.e. one positional argument per field) in order to function correctly with the keyword outer constructor.

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Done.

@simonbyrne simonbyrne merged commit 0210b1d into master Oct 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@simonbyrne simonbyrne deleted the sb/kwdef branch Oct 5, 2018

@mauro3

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

So this will be in 1.1, right?

@StefanKarpinski StefanKarpinski added this to the 1.1 milestone Oct 5, 2018

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Yes, though it shouldn't be breaking, so we could backport if you wanted it?

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Features don't go in point releases.

fredrikekre added a commit that referenced this pull request Nov 30, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 1, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 3, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 4, 2018

News and compat annotation for #29316
(at-kwdef for parametric structs, and structs with supertypes).

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Addition of NEWS and compat admonitions for important
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

fredrikekre added a commit that referenced this pull request Dec 5, 2018

Compat admonitions and NEWS for Julia 1.1 (#30230)
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>

@tpapp tpapp referenced this pull request Jan 3, 2019

Open

extended at-kwdef #638

@mauro3 mauro3 referenced this pull request Mar 11, 2019

Open

Nested structs fails #96

@simonbyrne

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

I would argue this is a bugfix rather than a feature. (from https://discourse.julialang.org/t/kwdef-not-working-for-parametric-struct/25286/4)

@fredrikekre

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

It's not even exported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.