<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    OK. Approved by me .. but .. it would be good if you can point out
    any other cases<br>
    where you think we can compatibly make changes to get rid of the
    need for suppressing this new warning.<br>
    <br>
    -phil<br>
    <br>
    On 9/23/19, 12:54 PM, Joe Darcy wrote:
    <blockquote
      cite="mid:7b7d3cdc-99b1-93be-b5b6-d6ee18aecf6c@oracle.com"
      type="cite">
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
      <p>Hi Phil,</p>
      <div class="moz-cite-prefix">On 9/22/2019 1:25 PM, Philip Race
        wrote:<br>
      </div>
      <blockquote type="cite" cite="mid:5D87D8B1.2070708@oracle.com">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        <br>
        <meta http-equiv="content-type" content="text/html;
          charset=UTF-8">
        <pre><span class="new">+        @SuppressWarnings("serial") // Not statically typed as Serializable

So is the comment being used to distinguish this overloading of what "serial" means ?
Why not introduce a new warning category ?</span></pre>
      </blockquote>
      <p><br>
      </p>
      <p>The -Xlint:serial check in javac has had an operational meaning
        of "does a serializable type define a serialVersionUID?" The
        work-in-progress is aiming to expand this to cover other aspect
        of declaring serializable (and externalizable) types.</p>
      <p>It would be possible to put the new checks in their own
        category, but that would limit their use and some of new checks
        find what are most likely semantic errors, such as declaring a
        serialVersionUID in an enum type, which gets silently ignored.<br>
      </p>
      <p><br>
      </p>
      <blockquote type="cite" cite="mid:5D87D8B1.2070708@oracle.com">
        <pre><span class="new">
</span></pre>
        Randomly looking at <br>
        ====<br>
        src/java.desktop/share/classes/java/awt/Container.java<br>
        <br>
        @@ -3849,10 +3849,11 @@<br>
         <br>
                 /**<br>
                  * The handler to fire {@code PropertyChange}<br>
                  * when children are added or removed<br>
                  */<br>
        +        @SuppressWarnings("serial") // Not statically typed as
        Serializable<br>
                 protected ContainerListener accessibleContainerHandler
        = null;<br>
        ===<br>
        <br>
        I see that Container has a writeObject method which doesn't
        write this field, so it is effectively transient.<br>
        <br>
        I recognise that it is difficult for the compiler to figure this
        out, so perhaps there should just be a policy<br>
        not to check classes that have writeObject methods ?<br>
      </blockquote>
      <p><br>
      </p>
      <p>Yes, it is not feasible for this level of analysis to decode
        the semantics of writeObject and related methods. The analysis
        does skip over classes using serialPersistentFields, which is an
        alternative mechanism to indicate which fields are used for
        serialization.</p>
      <p>In terms of possible false positives, I think it is acceptable
        to keep doing the checks in the presence of a writeObject method
        since a writeObject can be used to make alterations to
        serialization process other than changing the set of fields
        written out.<br>
      </p>
      <p><br>
      </p>
      <blockquote type="cite" cite="mid:5D87D8B1.2070708@oracle.com"> <br>
        Also in such a case, would it be an effectively compatible
        change to add transient to the field, so that<br>
        it presumably would no longer need this warning.<br>
      </blockquote>
      <p>And the class does define a serialVersionUID so adding
        transient to the field should preserve serial compatibility.<br>
      </p>
      <p><br>
      </p>
      <blockquote type="cite" cite="mid:5D87D8B1.2070708@oracle.com"> <br>
        I haven't looked but presumably there could be other such cases.<br>
        <br>
        Will you be filing bugs for all the fixable cases ?<br>
      </blockquote>
      <p>Someone should ;-)</p>
      <p>To make sure my intentions are clear, nothing in this overall
        cleanup effort should be construed as seeking to assume
        ownership of all the serialization in the JDK. The primary
        ownership will remain with the component team in question. The
        new checks are meant to the an aid, especially to writing new
        serializable types, while also prompting some examination of the
        existing types in an effort to allow the warning to enabled by
        default  in the build.</p>
      <p>Thanks,<br>
      </p>
      <p>-Joe</p>
      <p><br>
      </p>
      <blockquote type="cite" cite="mid:5D87D8B1.2070708@oracle.com"> <br>
        -phil<br>
        <br>
        On 9/21/19, 12:48 PM, Joe Darcy wrote:
        <blockquote
          cite="mid:ed084e59-bebd-714e-d8af-893115f0e6b3@oracle.com"
          type="cite">
          <meta http-equiv="content-type" content="text/html;
            charset=UTF-8">
          <p>Hello,</p>
          <p>Quick background, I'm working on expanding the compile-time
            serialization checks of javac's -Xlint:serial option. Ahead
            of that work going back, I'm analyzing the JDK sources and
            plan to pre-suppress the coming-soon new warnings, fixing or
            at least filing follow-up bugs for any problems that are
            found. Corresponding suppression bugs are already out for
            review against core libs (JDK-8231202) and security libs
            (JDK-8231262).</p>
          <p>The new check in development is if a serializable class has
            an instance field that is not declared to be a serializable
            type. This might actually be fine in practice, such as if
            the field in question always points to a serializable object
            at runtime, but it is arguably worth noting as an item of
            potential concern. This check is skipped if the class using
            the serialPersistentFields mechanism.</p>
          <p>For the client libs, the webrev with the new
            @SuppressedWarnings annotations is:<br>
          </p>
          <p>    JDK-8231334: Suppress warnings on non-serializable
            instance fields in client libs serializable classes <br>
                <a moz-do-not-send="true" class="moz-txt-link-freetext"
              href="http://cr.openjdk.java.net/%7Edarcy/8231334.0/">http://cr.openjdk.java.net/~darcy/8231334.0/</a></p>
          <p>The changes are mostly in awt, but also some in beans, a
            few in printing, and one in sound.<br>
          </p>
          <p>As discussed with Phil off-line, the new checks also found
            an existing known issue, the auxiliary class <span
              class="c-message__body" dir="auto" data-qa="message-text">java.awt.ImageMediaEntry

              declared in </span><span class="c-message__body"
              dir="auto" data-qa="message-text"><span
                class="c-message__body" dir="auto"
                data-qa="message-text">MediaTracker.java is not
                serializable/deserializable in practice (JDK-4397681).</span></span></p>
          <p><span class="c-message__body" dir="auto"
              data-qa="message-text"><span class="c-message__body"
                dir="auto" data-qa="message-text">Thanks,</span></span></p>
          <p><span class="c-message__body" dir="auto"
              data-qa="message-text"><span class="c-message__body"
                dir="auto" data-qa="message-text">-Joe<br>
              </span></span></p>
        </blockquote>
      </blockquote>
    </blockquote>
  </body>
</html>