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

Fix compile in Scala 2.10.1+ #1509

Merged
merged 1 commit into from Jan 16, 2014
Merged

Fix compile in Scala 2.10.1+ #1509

merged 1 commit into from Jan 16, 2014

Conversation

pbrant
Copy link
Member

@pbrant pbrant commented Dec 20, 2013

Scala 2.10.1 includes a type checking bug fix that causes compilation to
fail. Add casts to work around this (effectively restoring the previous
behavior).

Scala 2.10.1 includes a type checking bug fix that causes compilation to
fail.  Add casts to work around this (effectively restoring the previous
behavior).
@@ -360,7 +360,7 @@ trait AbstractScreen extends Factory {
case AVal(v: (T => List[FieldError])) => v
},
stuff.toList.collect {
case AFilter(v) => v
case AFilter(v) => v.asInstanceOf[T => T]
Copy link
Member

Choose a reason for hiding this comment

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

So, do we want to add an if isInstanceOf[T => T] to these case statements as well? As such, this will throw an exception if it can't fit into the type. May not be a horrible idea to log a warning or something as well...

Copy link
Member

Choose a reason for hiding this comment

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

I'm down for the isInstanceOf check, though it's worth mentioning that AFilter should already be providing a compiler check for the type. I think the only real concern here is reflection or something similarly weird that would ignore the compiler and run in a type-erased context could theoretically send in something other than what's expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler verification on AFilter construction isn't sufficient. 2.10.0
and earlier didn't complain and happily assumed T => T. 2.10.1+ does
complain. See https://issues.scala-lang.org/browse/SI-5189 . The cast is
basically restoring the pre-2.10.1 behavior (which is wrong, but mostly
good enough).

On Fri, Dec 20, 2013 at 3:17 PM, Antonio Salazar Cardozo <
notifications@github.com> wrote:

In web/webkit/src/main/scala/net/liftweb/http/LiftScreen.scala:

@@ -360,7 +360,7 @@ trait AbstractScreen extends Factory {
case AVal(v: (T => List[FieldError])) => v
},
stuff.toList.collect {

  •    case AFilter(v) => v
    
  •    case AFilter(v) => v.asInstanceOf[T => T]
    

I'm down for the isInstanceOf check, though it's worth mentioning that
AFilter should already be providing a compiler check for the type. I
think the only real concern here is reflection or something similarly weird
that would ignore the compiler and run in a type-erased context could
theoretically send in something other than what's expected.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1509/files#r8500039
.

Copy link
Member

Choose a reason for hiding this comment

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

I realize that. I'm not saying asInstanceOf isn't necessary, I'm saying isInstanceOf might be too much, depending on whether we view this as dealing with something we know but the compiler doesn't, or whether we view this as dealing with the potential for someone to get around the compiler to give us bad data :)

Copy link
Member

Choose a reason for hiding this comment

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

I'll let you guys decide on this one, once you all agree, just let me know and I'll press the green merge button (or any of you could do it too :) )

@Shadowfiend
Copy link
Member

Is this ready to go, @pbrant ? I don't know if you decided either way on the isInstanceOf check above.

@pbrant
Copy link
Member Author

pbrant commented Jan 16, 2014

I don't have any better ideas (i.e. I think it's good to go).

On Thu, Jan 16, 2014 at 3:38 PM, Antonio Salazar Cardozo <
notifications@github.com> wrote:

Is this ready to go, @pbrant https://github.com/pbrant ? I don't know
if you decided either way on the isInstanceOf check above.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1509#issuecomment-32473676
.

Shadowfiend added a commit that referenced this pull request Jan 16, 2014
Fix compile in Scala 2.10.1+

Scala 2.10.1 included a type checking bug fix that caused compilation to
fail for AFilter pattern matches. Add casts to work around this (effectively
restoring the previous behavior of scalac, but explicitly).
@Shadowfiend Shadowfiend merged commit fd38c7a into master Jan 16, 2014
@Shadowfiend Shadowfiend deleted the pmb_liftscreenfix branch January 16, 2014 14:51
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.

None yet

4 participants