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

Allow custom `apply` and `unapply` methods in case class companions #5730

Merged
merged 3 commits into from Mar 3, 2017

Conversation

@adriaanm
Copy link
Member

commented Feb 23, 2017

Don't emit a synthetic apply (or unapply) when it would
clash with an existing one. This allows e.g., a private apply,
along with a case class with a private constructor.

We have to retract the synthetic method in a pretty roundabout way,
as we need the other methods and the owner to be completed already.
Unless we have to complete the synthetic apply while completing
the user-defined one, this should not be a problem. If this does
happen, this implies there's a cycle in computing the user-defined
signature and the synthetic one, which is not allowed.

Community build: https://scala-ci.typesafe.com/job/scala-2.11.x-integrate-community-build/541/console

@adriaanm adriaanm added this to the 2.11.9 milestone Feb 23, 2017

@adriaanm adriaanm changed the title wip: allow user-defined apply/unapply method in case class companion wip: allow user-defined apply/unapply method in case class companion [ci: last-only] Feb 23, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2017

/rebuild

@adriaanm adriaanm force-pushed the adriaanm:userdefined-apply-211 branch 2 times, most recently from ddebd54 to f83aa5c Feb 23, 2017

@adriaanm adriaanm changed the title wip: allow user-defined apply/unapply method in case class companion [ci: last-only] allow user-defined apply/unapply method in case class companion [ci: last-only] Feb 24, 2017

@adriaanm adriaanm requested a review from retronym Feb 24, 2017

adriaanm added a commit to adriaanm/paradise that referenced this pull request Feb 24, 2017

adriaanm added a commit to adriaanm/community-builds that referenced this pull request Feb 24, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

Community build with forked paradise: https://scala-ci.typesafe.com/job/scala-2.11.x-integrate-community-build/535/console. Ok except for play-twirl, which seems to fail for an unrelated reason (macro compiled with older version of scala?? /cc @SethTisue, does this sound familiar?)

@adriaanm adriaanm changed the title allow user-defined apply/unapply method in case class companion [ci: last-only] Allow user-defined apply/unapply method in case class companion Feb 24, 2017

@adriaanm adriaanm force-pushed the adriaanm:userdefined-apply-211 branch from f83aa5c to a96472a Feb 24, 2017

// <synthetic> <case> def unapply[Ts](x: C[Ts]) = <ret-val>
// where <ret-val> is the caseClassUnapplyReturnValue of class C (see UnApplies.scala)
//
// This last condition requires special care, as we need to detect overloading without forcing too

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 24, 2017

Author Member

TODO: finish my sentence

@adriaanm adriaanm requested a review from lrytz Feb 24, 2017

@lrytz
Copy link
Member

left a comment

Looks very good!

There's an empty commit (676ec16).

}
else {
m = assignAndEnterSymbol(tree)
enterInScope(assignMemberSymbol(tree))

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 24, 2017

Member

This is another candidate for "enterInScope may return existing symbol? In interactive setting?"

If this happens (enterInScope returns a different symbol), it looks like tree.symbol no longer holds the correct symbol. Not sure if this causes issues. In any case, this commit doesn't make things worse.

}
case class ClashOverloadNoSig private (x: Int)

// needs sig because it's recursive

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 24, 2017

Member

error message would ideally also be more clear in this case ("recursive method needs result type"), but the cure is the same.

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

I'll see about moving some of the more ambitious changes over to 2.12.2. Is there anything before 09c35ae (incl) that you're concerned about?

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2017

On second thought, I'll just make the minimal changes here and move the cleanups to 2.12

@lrytz

This comment has been minimized.

Copy link
Member

commented Feb 24, 2017

It LGTM, but agree it's a good idea to make minimal changes in 2.11.

@adriaanm adriaanm force-pushed the adriaanm:userdefined-apply-211 branch from a96472a to 9f03f5f Feb 25, 2017

Allow user-defined `[un]apply` in case companion
Don't emit a synthetic `apply` (or `unapply`) when it would
clash with an existing one. This allows e.g., a `private apply`,
along with a `case class` with a `private` constructor.

We have to retract the synthetic method in a pretty roundabout way,
as we need the other methods and the owner to be completed already.
Unless we have to complete the synthetic `apply` while completing
the user-defined one, this should not be a problem. If this does
happen, this implies there's a cycle in computing the user-defined
signature and the synthetic one, which is not allowed.

@adriaanm adriaanm force-pushed the adriaanm:userdefined-apply-211 branch from 9f03f5f to 894b027 Feb 28, 2017

adriaanm added a commit to adriaanm/community-builds that referenced this pull request Feb 28, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

Ready for final 👀 .

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@SethTisue

This comment has been minimized.

Copy link
Member

commented Feb 28, 2017

Does this need a spec update?

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

True:

  • If a companion object cc exists already, no new object is created, but the apply and unapply methods are added to the existing object instead, unless the object already has a matching definition.
Clarify spec of interaction of existing vs synthetic apply/unapply
When matching user-defined apply/unapply members exist in a
case class's companion object, don't add clashing synthetic ones.
@lrytz
Copy link
Member

left a comment

Looks good, just a few clarifications.


// If there's a same-named locked symbol, we're currently completing its signature.
// This means it (may) refer to us, and is thus either overloaded or recursive without a signature.
// rule out locked symbols from the owner.info.member call

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 28, 2017

Member

Tricky stuff.. so if there's a locked sym in the same scope, the current sym is marked error. Does that cause the error to be reported? Why do we want to avoid the owner.info.member member call in this case?

Could there be a similar situation when there's a locked symbol not in the same scope, but inherited?

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 28, 2017

Author Member

member will need the info of the symbols its collecting in an OverloadedType (to see if they match). Calling info on those locked symbols will yield a TypeError to indicate the cycle, causing our stack to be unwound.

sym setInfo ErrorType
sym setFlag IS_ERROR

// Don't unlink in an error situation to generate less confusing error messages.

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 28, 2017

Member

It's not obvious to me why/how this impacts error reporting..

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 28, 2017

Author Member

Unlinking when there actually isn't a clash would result in the errors talking about recursive methods (since symWasOverloaded is false) instead of overloaded methods, flipping the wrongness of the error message to the other side.

    private def symWasOverloaded(sym: Symbol) = sym.owner.isClass && sym.owner.info.member(sym.name).isOverloaded

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 28, 2017

Author Member

If we could be more precise in determining whether there is a clash (by partially completing the method info in methodSig with a wildcard for the result type when it's inferred), we could make the error messages more accurate. Looking into that for 2.12

else
sym setInfo completerOf(tree)
}
// copy/apply/unapply synthetics are added using the addIfMissing mechanism,

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 28, 2017

Member

addIfMissing is no longer in this PR

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 28, 2017

Author Member

good catch

case class ClashOverloadNoSig private(x: Int)

object ClashRecNoSig {
// error: recursive method apply needs result type

This comment has been minimized.

Copy link
@lrytz

lrytz Feb 28, 2017

Member

The actual report says overloaded method apply needs result type, as you explain in the lengthy comment in Namers. So the behavior is fine, just the comment here is out of synch.

This comment has been minimized.

Copy link
@adriaanm

adriaanm Feb 28, 2017

Author Member

fixed

@fommil

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

I really like this, it's stung me a couple of times in the past.

@fommil

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

btw, somewhat related, a GSoC proposal I'm putting together for this year: https://github.com/scala/scala-lang/blob/master/gsoc/2017.md#case-classes-a-la-carte-with-scalameta

@adriaanm adriaanm force-pushed the adriaanm:userdefined-apply-211 branch from ad5953a to 6158490 Mar 2, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2017

Well then, looks like this should be the one 🤞

@retronym

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

When I reviewed the last iteration of this change, I mistakenly thought that the custom FindMember implementation replaced the entire call to .members, rather than just the search for the existence of a locked decl. That's why I thought it was a mistake to short circuit.

This is the code I'd imagined at the time that joins these two parts into a single member walk:

https://github.com/retronym/scala/tree/review/5730

It also avoids completing infos of members over trees that don't match the arity.

That said, I'm happy enough with this change as it is. Feel free to take some ideas from my branch if you further rework this on the 2.12.x branch, but I'm giving this the 👍 here.

@retronym retronym closed this Mar 3, 2017

@retronym retronym reopened this Mar 3, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

Thanks! For the next refinement, I'd prefer to enrich the type completer interface so that you can ask more directly for partial completions (e.g., just the types you can determine without looking at RHS, just the type params, ...)

@adriaanm adriaanm merged commit 3f437a2 into scala:2.11.x Mar 3, 2017

6 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [2801] SUCCESS. Took 15 s.
Details
validate-main [3114] SUCCESS. Took 120 min.
Details
validate-publish-core [3217] SUCCESS. Took 4 min.
Details
validate-test [2273] SUCCESS. Took 109 min.
Details
@retronym

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

Sounds good. e.g. we could add a method like schematicType that gives us the shape of the method type with wildcards. To be useful, we've have to change a few other places to use them, notable the checks for overriden members in namers, such as:

      val overridden = {
        val isConstr   = meth.isConstructor
        if (isConstr || !methOwner.isClass) NoSymbol else overriddenSymbol(methResTp)
      }
      val hasDefaults = mexists(vparamss)(_.symbol.hasDefault) || mexists(overridden.paramss)(_.hasDefault)
      if (hasDefaults)
        addDefaultGetters(meth, ddef, vparamss, tparams, overridden)
@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2017

adriaanm added a commit to adriaanm/scala that referenced this pull request Apr 5, 2017

`CompleterWrapper` delegates `typeParams`.
Fixes the problem reported with scala#5730 by xuwei-k in scala/scala-dev#352.

adriaanm added a commit to adriaanm/scala that referenced this pull request Apr 5, 2017

`CompleterWrapper` delegates `typeParams`.
Fixes the problem reported with scala#5730 by xuwei-k in scala/scala-dev#352.

The problem was already present before the introduction of
`applyUnapplyMethodCompleter`, as 63f7b35 (in scala#5294) introduced
a similar bug where the `PolyTypeCompleter`'s `typeParams` override
was masked.

adriaanm added a commit to adriaanm/scala that referenced this pull request Apr 6, 2017

WIP: `CompleterWrapper` delegates `typeParams`.
TODO: implement Lukas's observation and use CompleterWrapper for
other completers for polymorphic symbols

Fixes the problem reported with scala#5730 by xuwei-k in scala/scala-dev#352.

The problem was already present before the introduction of
`applyUnapplyMethodCompleter`, as 63f7b35 (in scala#5294) introduced
a similar bug where the `PolyTypeCompleter`'s `typeParams` override
was masked.

adriaanm added a commit to adriaanm/scala that referenced this pull request Apr 6, 2017

`CompleterWrapper` delegates `typeParams`.
Fixes the problem reported with scala#5730 by xuwei-k in scala/scala-dev#352.

The problem was already present before the introduction of
`applyUnapplyMethodCompleter`, as 63f7b35 (in scala#5294) introduced
a similar bug where the `PolyTypeCompleter`'s `typeParams` override
was masked.

adriaanm added a commit to adriaanm/scala that referenced this pull request Apr 8, 2017

Merge 2.11.x into 2.12.x
Skip to 3f437a2 Merge pull request scala#5730

@SethTisue SethTisue modified the milestones: 2.11.11, 2.11.9 May 1, 2017

@SethTisue SethTisue changed the title Allow user-defined apply/unapply method in case class companion Allow user-defined apply/unapply methods in case class companion May 1, 2017

@SethTisue SethTisue changed the title Allow user-defined apply/unapply methods in case class companion Allow custom `apply` and `unapply` methods in case class companions May 1, 2017

@naferx naferx referenced this pull request Dec 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.