On Tue, 22 Dec 2020 20:17:59 GMT, Phil Race <p...@openjdk.org> wrote:
>But all of them need to be re-examined rather than just blindly updating them >as some tool suggests. I agree. I actually did careful investigation of all my changes. As you can see SpotBugs reported much more than I actually propose to fix. I reverted most controversal changes from this PR. I hoped that reviewers could help me to clarify controversal changes and start discussion about unclear places. I will go through my changes and my thoughts about it: ### 1. `DataBufferUShort` explicit NPE in constructors  Behaviour of `DataBufferUShort` constructors in case if `dataArray == null` is the same as in original code, as in the changed code, as in _hypothetical code_ where we removed dereference before `null` check. This behavior - is NullPointerException This code is in original form, as it was uploaded into open source under https://bugs.openjdk.java.net/browse/JDK-6662775 So there is no chance that there are consumers of this code which expect _NPE with this specific message_ `dataArray is null`. ### 2. Dereference of parameter `JComponent c` in method `getPreferredSize` in classes `BasicToolTipUI`, `SynthToolTipUI`  This dereference was there from original upload of OpenJDK. While NPE is not specified in javadoc of method `javax.swing.plaf.ComponentUI#getPreferredSize`, most of implementations do not check nullness of passed parameter. As I know it matches with general Swing behaviour about null tolerance. ### 3. Dereference of field `stack` in `javax.swing.text.html.parser.Parser#ignoreElement`  This dereference was there from original upload of OpenJDK. Method `ignoreElement` is called only from method `legalElementContext` which has `null` check at the start: // Deal with the empty stack if (stack == null) { // System.out.println("-- stack is empty"); if (elem != dtd.html) { // System.out.println("-- pushing html"); startTag(makeTag(dtd.html, true)); return legalElementContext(elem); } return true; } ### 4. Null checks of paragraph `javax.swing.text.Element` after `for` cycle in 3 methods: `javax.swing.text.html.AccessibleHTML.TextElementInfo.TextAccessibleContext#getParagraphElement`, `javax.swing.text.DefaultStyledDocument#getParagraphElement`, `javax.swing.text.JTextComponent.AccessibleJTextComponent#getParagraphElement`  This methods have the same cycle before `null` check Element para; for (para = model.getDefaultRootElement(); ! para.isLeaf(); ) { int pos = para.getElementIndex(index); para = para.getElement(pos); } SpotBugs reported redundant `null` check of `para`, because dereference is alredy happened in `for` cycle condition: `! para.isLeaf()` This dereference and `null` check were there from original upload of OpenJDK. `para` is a result of `Element.getElement` method call. Since parameter `pos` is result of previous call to `Element.getElementIndex` - and this method, according to its javadoc, must return valid child element index for non-leafs. It means `Element.getElement` shouldn't return `null` values according to contracts. So even if some custom implementation returns `null` - it violates `Element` contract and NPE is expected. OpenJDK implementations of `Element` ### 5. Null check of in method `javax.swing.text.html.StyleSheet#getRule(HTML.Tag, Element)`  Variable `AttributeSet attr` is result of method `javax.swing.text.Element#getAttributes`. It is dereferenced and then `null` checked. This dereference was there from original upload of OpenJDK. Javadoc of method `javax.swing.text.Element#getAttributes` doesn't specify that it can return `null`. OpenJDK implementations never return `null`. Actually there is only one implementation `javax.swing.text.AbstractDocument.AbstractElement#getAttributes`: public AttributeSet getAttributes() { return this; } ### 6. `Component c = javax.swing.MenuElement.getComponent()` is `null`-checked in `javax.swing.JMenuBar#processBindingForKeyStrokeRecursive`  This dereference was there from original upload of OpenJDK. Variable `c` is checked before another condition `c instanceof JComponent` in the same `if` statement. `instanceof JComponent` do not return `true` for `null` values. So it's safe to remove. Javadoc of method `javax.swing.MenuElement.getComponent()` doesn't specify `null` as possible return value. All implementations inside OpenJDK return non-null value: `this` ### 7. Null check of `File fontFile` inside `sun.font.FileFont.CreatedFontFileDisposerRecord#dispose`  This field of `sun.font.FileFont.CreatedFontFileDisposerRecord` class initialized in constructor, which is called in one place `sun.font.FileFont#setFileToRemove`. `File file` is passed there from the caller method `sun.font.SunFontManager#createFont2D`. At the start method `sun.font.SunFontManager#createFont2D` there is dererefence: String fontFilePath = fontFile.getPath(); This internal method `sun.font.FontManager#createFont2D` never accepted `null` values since its introduction. PS. I tracked where original `fontFile` value could came from and found 2 public methods: a. `java.awt.Font#createFont(int, java.io.File)` - it does specify `NullPointerException` in it's javadoc b. `java.awt.Font#createFonts(java.io.File)` - it does *NOT* specify `NullPointerException` in it's javadoc. Perphaps it should be specified to be consistent. Anyway this fact is not directly related to removing redundant `null` check in `sun.font.FileFont.CreatedFontFileDisposerRecord#dispose`. Variable is dereferenced multiple times in code flow, before calling this method. ### 8. Null check of `bufferedImage` in `sun.print.PathGraphics#hasTransparentPixels`  This dereference was there from original upload of OpenJDK. Variable is dereferenced at the start of method. This method is called from 2 places: `sun.print.PSPathGraphics#drawImageToPlatform` and in `sun.awt.windows.WPathGraphics#drawImageToPlatform`. Both of this methods handle `null` values by themselves BufferedImage img = getBufferedImage(image); if (img == null) { return true; } ------------- PR: https://git.openjdk.java.net/jdk/pull/1869
