Skip to content

Don't abort on glb/lub of TypeVar#6338

Merged
adriaanm merged 1 commit intoscala:2.13.xfrom
joroKr21:glb-lub-typevar
Aug 8, 2018
Merged

Don't abort on glb/lub of TypeVar#6338
adriaanm merged 1 commit intoscala:2.13.xfrom
joroKr21:glb-lub-typevar

Conversation

@joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Feb 18, 2018

Instead throw a NoCommonType exception and catch it when solving
type variables to avoid a compiler crash when type inference
cannot satisfy all constraints.

Fixes scala/bug#10514 by turning the crash into an error message.

scala/bug#10519 and scala/bug#5559 also produce error messages,
but are not fixed IMO, because it looks like they should compile.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Feb 18, 2018
@joroKr21
Copy link
Member Author

joroKr21 commented Feb 18, 2018

/sync

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
adriaanm
adriaanm previously approved these changes Aug 3, 2018
View reviewed changes
adriaanm
adriaanm reviewed Aug 3, 2018
View reviewed changes
Copy link
Contributor

@adriaanm adriaanm left a comment

Choose a reason for hiding this comment

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

Let's check performance.

src/reflect/scala/reflect/internal/tpe/TypeConstraints.scala Outdated
} else {
if (depth.isAnyDepth) lub(tvar.constr.loBounds) else lub(tvar.constr.loBounds, depth)
}
} catch { case unsolvable: NoCommonType =>
Copy link
Contributor

@adriaanm adriaanm Aug 3, 2018

Choose a reason for hiding this comment

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

Hmm, I'm concerned about the performance impact. Can/should we handle this by returning an error result? /cc @retronym

@adriaanm adriaanm dismissed their stale review August 3, 2018 14:38

Second thoughts about performance.

@adriaanm
Copy link
Contributor

adriaanm commented Aug 7, 2018

Clearly the abort is wrong, but how about just ignoring this case? I'd be more comfortable turning the abort into a debuglog. Let's not introduce exception handling here.

Btw, smaller test case:

class C[T](x: T)
class Foo[F[_], G[_]](value: F[C[G[Foo[F, G]]]])

class Test {
  type Id[A] = A
  new Foo(Some(new C(new Foo[Option, Id](None))))
}

@adriaanm
Copy link
Contributor

adriaanm commented Aug 8, 2018

I hope you won't mind if I push -f to your branch to implement my suggestion. (Original sha: 5326029)

@joroKr21 @adriaanm
Don't abort on glb/lub of TypeVar; ignore it.
ac9c807 
Fixes scala/bug#10514 by turning the crash into an error message.
@adriaanm adriaanm merged commit e64564b into scala:2.13.x Aug 8, 2018
@SethTisue
Copy link
Member

SethTisue commented Aug 8, 2018

thank you Georgi!

@joroKr21
Copy link
Member Author

joroKr21 commented Aug 9, 2018

@adriaanm Thank you for amending my old PRs (or should I say sins). I haven't had much time lately. For some reason I thought that if we actually try to glb/lub of TypeVar the Universe would implode.

@adriaanm
Copy link
Contributor

adriaanm commented Aug 9, 2018

No worries, sorry for taking so long. It's intense stuff :-)

We actually still won't lub/glb the TypeVars -- they're now stripped by stripExistentialsAndTypeVars if there's no instance to replace them with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@adriaanm adriaanm adriaanm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adriaanm adriaanm

Labels

None yet

Projects

None yet

Milestone

2.13.0-M5

Development

Successfully merging this pull request may close these issues.

trying to do lub/glb of typevar ?G[Foo[?F,?G,?A]]

4 participants

@joroKr21 @adriaanm @SethTisue @scala-jenkins