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

[java] Regression with UnusedAssignment #4960

Open
uhafner opened this issue Apr 15, 2024 · 1 comment
Open

[java] Regression with UnusedAssignment #4960

uhafner opened this issue Apr 15, 2024 · 1 comment
Labels
a:false-positive PMD flags a piece of code that is not problematic

Comments

@uhafner
Copy link

uhafner commented Apr 15, 2024

Affects PMD Version:
7.0.0

Rule:
UnusedAssignment

Please provide the rule name and a link to the rule documentation:
(https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#unusedassignment)

Description:

When a field is changed at the end of a method and is not used again in that method, than PMD reports that the written value is unused. Even though the value is used in another method that is called later in the processing.

Code Sample demonstrating the issue:

See original code and suppression and checks result in:

    /**
     * Handles the parsing of a translation file from Qt.
     */
    static class QtTranslationSaxParser extends DefaultHandler {
        private static final String CONTEXT = "context";
        private static final String CONTEXT_NAME = "name";
        private static final String MESSAGE = "message";
        private static final String MESSAGE_NUMERUS_ATTRIBUTE = "numerus";
        private static final String MESSAGE_NUMERUS_ENABLED_VALUE = "yes";
        private static final String NUMERUSFORM = "numerusform";
        private static final String ROOT = "TS";
        private static final String SOURCE = "source";
        private static final String TRANSLATION = "translation";
        private static final String TRANSLATION_ATTR_TYPE = "type";

        // The locator will be initialized within setDocumentLocator, which will be called by SAXParser.
        @SuppressFBWarnings("NP_NONNULL_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR")
        @SuppressWarnings("NullAway")
        private Locator documentLocator;

        private final Report report;
        private final IssueBuilder builder = new IssueBuilder();
        private final Deque<String> elementTypeStack = new ArrayDeque<>();
        private final Map<String, String> expectedElementTypeParents = new HashMap<>();
        private String contextName = "";
        private String sourceValue = "";
        private String translationType = "";
        private boolean hasTranslatedString = false;
        private boolean translationTagFound = false;
        private boolean messageIsNumerus = false;
        private int lastColumnNumber;

        /**
         * Creates a new instance of {@link QtTranslationSaxParser}.
         *
         * @param report
         *         the issues
         * @param fileName
         *         path to the translation file
         */
        QtTranslationSaxParser(final Report report, final String fileName) {
            super();

            expectedElementTypeParents.put(ROOT, null);
            expectedElementTypeParents.put(CONTEXT, ROOT);
            expectedElementTypeParents.put(CONTEXT_NAME, CONTEXT);
            expectedElementTypeParents.put(MESSAGE, CONTEXT);
            expectedElementTypeParents.put(NUMERUSFORM, TRANSLATION);
            expectedElementTypeParents.put(TRANSLATION, MESSAGE);
            expectedElementTypeParents.put(SOURCE, MESSAGE);

            this.report = report;
            builder.setFileName(fileName);
        }

        @Override
        public void setDocumentLocator(final Locator locator) {
            documentLocator = locator;
        }

        @Override
        public void startElement(final String namespaceURI,
                final String localName, final String key, final Attributes atts) {
            verifyElementTypeRelation(key);

            elementTypeStack.push(key);

            switch (key) {
                case CONTEXT_NAME:
                    throwParsingExceptionBecauseOfDuplicatedOccurrence(!contextName.isEmpty(), key);
                    break;
                case SOURCE:
                    throwParsingExceptionBecauseOfDuplicatedOccurrence(!sourceValue.isEmpty(), key);
                    break;
                case TRANSLATION:
                    throwParsingExceptionBecauseOfDuplicatedOccurrence(translationTagFound, key);
                    translationTagFound = true;

                    translationType = atts.getValue(TRANSLATION_ATTR_TYPE);
                    if (translationType != null) {
                        applyTranslationType();
                    }
                    break;
                case MESSAGE:
                    builder.setLineStart(documentLocator.getLineNumber());
                    builder.setColumnStart(lastColumnNumber);

                    messageIsNumerus = MESSAGE_NUMERUS_ENABLED_VALUE.equals(atts.getValue(MESSAGE_NUMERUS_ATTRIBUTE));
                    break;
                case NUMERUSFORM:
                    if (!messageIsNumerus) {
                        throw new ParsingException(
                                "Element type \"%s\" is allowed only if the attribute \"%s\" within the parent element \"%s\" is set to \"%s\" (line %d).",
                                NUMERUSFORM,
                                MESSAGE_NUMERUS_ATTRIBUTE,
                                MESSAGE,
                                MESSAGE_NUMERUS_ENABLED_VALUE,
                                documentLocator.getLineNumber());
                    }
                    break;
                default:
                    break;
            }

            lastColumnNumber = documentLocator.getColumnNumber();
        }

        @Override
        public void endElement(final String uri, final String localName, final String qName) {
            elementTypeStack.pop();
            lastColumnNumber = documentLocator.getColumnNumber();

            if (CONTEXT.equals(qName)) {
                contextName = "";
                return;
            }

            if (!MESSAGE.equals(qName)) {
                return;
            }

            throwParsingExceptionBecauseOfMissingElementType(contextName.isEmpty(), CONTEXT_NAME);
            throwParsingExceptionBecauseOfMissingElementType(sourceValue.isEmpty(), SOURCE);
            throwParsingExceptionBecauseOfMissingElementType(!translationTagFound, TRANSLATION);

            if (translationType != null) {
                // In case the translation type is "unfinished" and we found a translation,
                // change the type to TRANSLATION_TYPE_UNFINISHED_NOT_EMPTY.
                // This case is not handled within {@link #applyTranslationType(String)}
                // because it is not an official state within a translation file of Qt.
                if (TRANSLATION_TYPE_UNFINISHED.equals(translationType) && hasTranslatedString) {
                    builder.setSeverity(Severity.WARNING_LOW);
                    builder.setMessage(TRANSLATION_TYPE_UNFINISHED_NOT_EMPTY_MESSAGE);
                    builder.setCategory(TRANSLATION_TYPE_UNFINISHED_NOT_EMPTY);
                }

                builder.setLineEnd(documentLocator.getLineNumber());
                builder.setColumnEnd(documentLocator.getColumnNumber());

                report.add(builder.build());
            }
            // prepare for next message block
            hasTranslatedString = false;
            translationTagFound = false;
            sourceValue = "";
        }

        @Override 
        @SuppressWarnings("PMD.UnusedAssignment") // false positive in PMD 7.0.0
        public void characters(final char[] ch, final int start, final int length) {
            lastColumnNumber = documentLocator.getColumnNumber();
            if (CONTEXT_NAME.equals(elementTypeStack.getFirst())) {
                contextName = new String(ch, start, length);
            }
            if (SOURCE.equals(elementTypeStack.getFirst())) {
                sourceValue = new String(ch, start, length);
            }
            if (TRANSLATION.equals(elementTypeStack.getFirst())) {
                // In case the message is numerus, the decision is made by a separate element.
                // Keep the value to true if it is already set.
                hasTranslatedString |= !messageIsNumerus;
            }
            if (NUMERUSFORM.equals(elementTypeStack.getFirst())) {
                hasTranslatedString = true;
            }
        }

        private void verifyElementTypeRelation(final String element) {
            String parent = expectedElementTypeParents.getOrDefault(element, "");
            if (parent == null) {
                if (!elementTypeStack.isEmpty()) {
                    throw new ParsingException("Element type \"%s\" does not expect to be a root element (line %d).",
                            element,
                            documentLocator.getLineNumber());
                }
                return;
            }

            if (!parent.isEmpty() && !elementTypeStack.getFirst().equals(parent)) {
                throw new ParsingException(
                        "Element type \"%s\" expects to be a child element of element type \"%s\" (line %d).",
                        element,
                        parent,
                        documentLocator.getLineNumber());
            }
        }

        @SuppressWarnings("NullAway")
        private void throwParsingExceptionBecauseOfDuplicatedOccurrence(final boolean shouldThrow,
                final String element) {
            if (shouldThrow) {
                throw new ParsingException(
                        "Element type \"%s\" can be used only once within element type \"%s\" (line %d).",
                        element,
                        expectedElementTypeParents.get(element),
                        documentLocator.getLineNumber());
            }
        }

        @SuppressWarnings("NullAway")
        private void throwParsingExceptionBecauseOfMissingElementType(final boolean shouldThrow, final String element) {
            if (shouldThrow) {
                throw new ParsingException(
                        "Missing or empty element type \"%s\" within element type \"%s\" (line %d).",
                        element,
                        expectedElementTypeParents.get(element),
                        documentLocator.getLineNumber());
            }
        }

        private void applyTranslationType() {
            switch (translationType) {
                case TRANSLATION_TYPE_OBSOLETE:
                    builder.setSeverity(Severity.WARNING_NORMAL);
                    builder.setMessage(TRANSLATION_TYPE_OBSOLETE_MESSAGE);
                    break;
                case TRANSLATION_TYPE_UNFINISHED:
                    builder.setSeverity(Severity.WARNING_LOW);
                    builder.setMessage(TRANSLATION_TYPE_UNFINISHED_MESSAGE);
                    break;
                case TRANSLATION_TYPE_VANISHED:
                    builder.setSeverity(Severity.WARNING_NORMAL);
                    builder.setMessage(TRANSLATION_TYPE_VANISHED_MESSAGE);
                    break;
                default:
                    throw new ParsingException("Unknown translation state \"%s\" (line %d).",
                            translationType,
                            documentLocator.getLineNumber());
            }
            builder.setCategory(translationType);
        }

        @Override
        public void endDocument() throws SAXException {
            builder.close();
        }
    }

Expected outcome:

PMD reports a violation at line ..., but that's wrong. That's a false positive.

PMD 6.x did not report a violation.

Running PMD through: [Maven]

@uhafner uhafner added the a:false-positive PMD flags a piece of code that is not problematic label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-positive PMD flags a piece of code that is not problematic
Projects
None yet
Development

No branches or pull requests

1 participant