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 for SerialVersionUID instability. #1673

Merged
merged 4 commits into from Dec 3, 2012

Conversation

Projects
None yet
4 participants
@paulp
Contributor

paulp commented Nov 26, 2012

Can hardly believe this has been broken for a decade or so, but
there it is - see test case. Four classes attempt to set their
SerialVersionUID to 13. One succeeds. No warnings or errors. The
output before this patch (for me anyway - your random numbers may
differ) was:

860336111422349646
13
8409527228024057943
-7852527872932878365

There was already code in place for rejecting annotations
with non-constant args when constant args are required, but
that check is only performed on ClassfileAnnotations, and
SerialVersionUID was a StaticAnnotation. Maybe people don't
reach for ClassfileAnnotation because of this giant warning
which I see no way to suppress:

warning: Implementation restriction: subclassing Classfile does
not make your annotation visible at runtime. If that is what you
want, you must write the annotation class in Java.

Why did I change the name of the field from uid to value?
If you don't use the name 'value', you have to name the argument
every time you use it, even if it's the only parameter. I didn't
relish breaking every usage of scala's @serialversionuid in the
known universe.

Fix for SerialVersionUID instability.
Can hardly believe this has been broken for a decade or so, but
there it is - see test case. Four classes attempt to set their
SerialVersionUID to 13. One succeeds. No warnings or errors. The
output before this patch (for me anyway - your random numbers may
differ) was:

  860336111422349646
  13
  8409527228024057943
  -7852527872932878365

There was already code in place for rejecting annotations
with non-constant args when constant args are required, but
that check is only performed on ClassfileAnnotations, and
SerialVersionUID was a StaticAnnotation. Maybe people don't
reach for ClassfileAnnotation because of this giant warning
which I see no way to suppress:

  warning: Implementation restriction: subclassing Classfile does
  not make your annotation visible at runtime. If that is what you
  want, you must write the annotation class in Java.

Why did I change the name of the field from uid to value?
If you don't use the name 'value', you have to name the argument
every time you use it, even if it's the only parameter. I didn't
relish breaking every usage of scala's @serialversionuid in the
known universe.
@paulp

This comment has been minimized.

Contributor

paulp commented Nov 26, 2012

Opened against master because it's presumably binary-incompatible.

@paulp

This comment has been minimized.

Contributor

paulp commented Nov 26, 2012

Review by anyone with a clue about serialization. @jsuereth, @lrytz ?

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1735/

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1735/

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

paulp added some commits Nov 26, 2012

Eliminate some one-arg asserts.
The only thing more fun than debugging non-deterministic
scaladoc crashes unrelated to one's change is doing so with
all one-argument asserts.
Account for existence of scala's ClassfileAnnotation.
Apparently this thing is not real well tested, as the
scaladoc code was written as if it does not exist.
@paulp

This comment has been minimized.

Contributor

paulp commented Nov 26, 2012

PLS REBUILD ALL

Disabled part of a test.
Hmmm, the giant blob of binary data embedded in a test
suddenly stopped working. What does one do in this spot.
@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1736/

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1736/

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

@scala-jenkins

This comment has been minimized.

scala-jenkins commented Nov 26, 2012

@paulp

This comment has been minimized.

Contributor

paulp commented Nov 30, 2012

Okay, review by @harrah, you must know something about serialization.

@harrah

This comment has been minimized.

Contributor

harrah commented Nov 30, 2012

LGTM

paulp added a commit that referenced this pull request Dec 3, 2012

Merge pull request #1673 from paulp/serialversionuid
Fix for SerialVersionUID instability.

@paulp paulp merged commit 4b2330b into scala:master Dec 3, 2012

retronym added a commit to retronym/scala that referenced this pull request May 5, 2014

SI-8459 Honour the @serialversionuid annotatation
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was
changed from a `StaticAnnotation` to `ClassFileAnnotation` in
order to avoid silently ignoring non-literal UIDs like:

    @serialversionuid(0 - 12345L) class C

And to flag non-constant UIDs:

    @serialversionuid("!!!".length)

While this indeed was fold constants, the change was incomplete.
The compiler API for reading the argument from a `ClassFileAnnoation`
is different, on must look for a `LiteralAnnotArg`, rather than a
`Literal`.

This commit:

  - amends the backend accordingly
  - removes relevant duplication between `GenASM` and `GenBCode`
  - tests that the static field is generated accordingly

This will mean that we will break deserialization of objects from
Scalal 2.11.0 that use this annotation.

retronym added a commit to retronym/scala that referenced this pull request May 5, 2014

SI-8549 Honour the @serialversionuid annotatation
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was
changed from a `StaticAnnotation` to `ClassFileAnnotation` in
order to avoid silently ignoring non-literal UIDs like:

    @serialversionuid(0 - 12345L) class C

And to flag non-constant UIDs:

    @serialversionuid("!!!".length)

While this indeed was fold constants, the change was incomplete.
The compiler API for reading the argument from a `ClassFileAnnoation`
is different, on must look for a `LiteralAnnotArg`, rather than a
`Literal`.

This commit:

  - amends the backend accordingly
  - removes relevant duplication between `GenASM` and `GenBCode`
  - tests that the static field is generated accordingly

This will mean that we will break deserialization of objects from
Scalal 2.11.0 that use this annotation.
@paulp

This comment has been minimized.

Contributor

paulp commented May 7, 2014

Did "I didn't relish breaking every usage of scala's @serialversionuid in the known universe" win any prestigious irony awards?

@som-snytt

This comment has been minimized.

Contributor

som-snytt commented May 7, 2014

"Known universe" was too restrictive a domain, so it was actually up for Understatement. But the other comment that "it's presumably binary-incompatible" won one of those technical awards they give out the day before the big event which no one attends except of a couple of stringers. I think the category was, "Irony in an observation that applies to everything except this." But next year they're starting a special category to recognize binary incompatibility; the statuette shows one hand washing another, with the inscription on the base: "mea paulpa."

lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014

SI-8960 Bring back the SerialVersionUID to anonymous function classes
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was
changed from a `StaticAnnotation` to `ClassFileAnnotation` in
order to enforce annotation arguments to be constants.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.

lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014

SI-8960 Bring back the SerialVersionUID to anonymous function classes
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was changed
from a `StaticAnnotation` to `ClassFileAnnotation` in order to enforce
annotation arguments to be constants. That was 2.11.0.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0 for
2.11.1.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.

lrytz added a commit to lrytz/scala that referenced this pull request Nov 5, 2014

SI-8960 Bring back the SerialVersionUID to anonymous function classes
In PR scala#1673 / 4267444, the annotation `SerialVersionId` was changed
from a `StaticAnnotation` to `ClassFileAnnotation` in order to enforce
annotation arguments to be constants. That was 2.11.0.

The ID value in the AnnotationInfo moved from `args` to `assocs`, but
the backend was not adjusted. This was fixed in PR scala#3711 / ecbc9d0 for
2.11.1.

Unfortunately, the synthetic AnnotationInfo that is added to anonymous
function classes still used the old constructor (`args` instead of
`assocs`), so extracting the value failed, and no field was added to
the classfile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment