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

SI-7296 Lifting the limit on case class arity #2305

Merged
merged 2 commits into from Mar 29, 2013

Conversation

Projects
None yet
4 participants
@retronym
Member

retronym commented Mar 25, 2013

The first commit fixes a crasher with the limit still imposed, that shows up with nested fat case classes.

The second lifts the limit. For case classes with > 22 params:

  • omit generation of unapply
  • pattern matching directly binds to the case accessors (as it would do for smaller case classes already)
  • the companion doesn't inherit FunctionN.

There was some understandable fear of the piecemeal when
I tabled this idea on scala-internals [1]. But I'd like
to persist as this limit is a needless source of pain for
anyone using case classes to bind to database, XML or JSON
schemata.

Review by @paulp @szeiger (for a Slick perspective), and by the Scala Meeting tomorrow.

[1] https://groups.google.com/forum/#!topic/scala-internals/RRu5bppi16Y

retronym added some commits Mar 25, 2013

SI-7296 Avoid crash with nested 23-param case class
The implementation restriction doesn't stop subsequent
typechecking in the same compilation unit from triggering
type completion which tries to synthesize the unapply
method.

This commit predicates generation of the unapply method
on having 22 or fewer parameters.
SI-7296 Remove arity limit for case classes
When venturing above the pre-ordained limit of twenty
two, `Companion extends FunctionN` and `Companion.unapply`
are sacrificed. But oh-so-many other case class features
work perfectly: equality/hashing/stringification, the apply
method, and even pattern matching (which already bypasses
unapply.)

There was some understandable fear of the piecemeal when
I tabled this idea on scala-internals [1]. But I'd like
to persist as this limit is a needless source of pain for
anyone using case classes to bind to database, XML or JSON
schemata.

[1] https://groups.google.com/forum/#!topic/scala-internals/RRu5bppi16Y
@szeiger

This comment has been minimized.

Show comment
Hide comment
@szeiger

szeiger Mar 25, 2013

Contributor

LGTM. This should have no impact on Slick at the moment but could be used to map case classes with higher arities once we can generate unboxed access paths that avoid the intermediate tuples.

Attempts to use large case classes when going through tupled APIs may lead to surprising error messages. I have opened SI-7299 for that.

Contributor

szeiger commented Mar 25, 2013

LGTM. This should have no impact on Slick at the moment but could be used to map case classes with higher arities once we can generate unboxed access paths that avoid the intermediate tuples.

Attempts to use large case classes when going through tupled APIs may lead to surprising error messages. I have opened SI-7299 for that.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Mar 25, 2013

Member

Good catch, I've sharpened up the error message in #2306.

Member

retronym commented Mar 25, 2013

Good catch, I've sharpened up the error message in #2306.

@ghost ghost assigned paulp Mar 25, 2013

@paulp

This comment has been minimized.

Show comment
Hide comment
@paulp

paulp Mar 25, 2013

Contributor

Is there any opportunity to warn about the differences from 22-and-under case classes without being annoyingly noisy? The enumerated distinctions seem like things which would turn up as "it doesn't compile anymore", not as hidden traps, so I'm not too fretty about it; but when there is a switch flipped at an utterly arbitrary number it feels like something somewhere ought to openly recognize the event, at the least if the user gives any sign of being interested in hearing about more things (e.g. -Xlint.)

Contributor

paulp commented Mar 25, 2013

Is there any opportunity to warn about the differences from 22-and-under case classes without being annoyingly noisy? The enumerated distinctions seem like things which would turn up as "it doesn't compile anymore", not as hidden traps, so I'm not too fretty about it; but when there is a switch flipped at an utterly arbitrary number it feels like something somewhere ought to openly recognize the event, at the least if the user gives any sign of being interested in hearing about more things (e.g. -Xlint.)

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Mar 25, 2013

Member

We could mandate the import of scala.language.largeCaseClasses. But I'm not really a fan of that, as you lose so little when you cross the line.

Member

retronym commented Mar 25, 2013

We could mandate the import of scala.language.largeCaseClasses. But I'm not really a fan of that, as you lose so little when you cross the line.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Mar 27, 2013

Member

LGTM

Member

adriaanm commented Mar 27, 2013

LGTM

adriaanm added a commit that referenced this pull request Mar 29, 2013

Merge pull request #2305 from retronym/ticket/7296-2
SI-7296 Lifting the limit on case class arity

@adriaanm adriaanm merged commit 7894c1b into scala:master Mar 29, 2013

1 check passed

default pr-checkin-per-commit Took 84 min.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment