Skip to content

Fix 1365: Fix bindings in patterns #1377

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

Merged
merged 2 commits into from
Jul 15, 2016
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 9, 2016

We need to compare pattern types with expected types in order
to derive knowledge about pattern-bound variables. This is done
using the mechanism of gadt bounds.

This is a subtle aspect of type inference that was so far missing from dotty. scalac does it
differently, I would argue in a more ad-hoc way.

Review by @DarkDimius.

We need to compare pattern types with expected types in order
to derive knowledge about pattern-bound variables. This is done
use the mechanism of gadt bounds.
if (!ctx.isAfterTyper) {
val setBefore = ctx.mode is Mode.GADTflexible
tpt1.tpe.<:<(pt)(ctx.addMode(Mode.GADTflexible))
if (!setBefore) ctx.retractMode(Mode.GADTflexible)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to rectractMode? ctx here is not fresh and addMode should create a new one instead of mutating existing one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from the GADT fix, where I didn't want to change the state of the context apart from relaxing the solver for GADTs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as @DarkDimius notes it's a new context that gets created, so this should not be a problem.

@DarkDimius
Copy link
Contributor

Otherwise LGTM

@DarkDimius
Copy link
Contributor

It would be also nice to add a test.

The dropped lines were both pure functions whose result is ignored.
@odersky
Copy link
Contributor Author

odersky commented Jul 15, 2016

@DarkDimius There is a test: pos/1365.scala. Should something else be added to it?

@DarkDimius
Copy link
Contributor

@odersky, Sorry for confusion, I've left a comment on a wrong PR. It should have been in #1393.

@odersky odersky merged commit dfa3280 into scala:master Jul 15, 2016
*/
private def newPatternBoundSym(name: Name, info: Type, pos: Position)(implicit ctx: Context) = {
val flags = if (name.isTypeName) BindDefinedType else EmptyFlags
val sym = ctx.newSymbol(ctx.owner, name, flags | Case, info, coord = pos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case is described as:

 /** A case class or its companion object */
final val Case = commonFlag(17, "case")
final val CaseClass = Case.toTypeFlags
final val CaseVal = Case.toTermFlags

so I don't understand what it's supposed to do here

@allanrenucci allanrenucci deleted the #1365 branch December 14, 2017 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants