diff --git a/Gemfile.lock b/Gemfile.lock index a95b9119a8..28d16589fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -31,9 +31,9 @@ GEM multipart-post (>= 1.2, < 3) faraday-http-cache (1.3.1) faraday (~> 0.8) - fugit (1.3.5) + fugit (1.3.6) et-orbi (~> 1.1, >= 1.1.8) - raabro (~> 1.1) + raabro (~> 1.3) git (1.7.0) rchardet (~> 1.8) kramdown (1.17.0) @@ -42,13 +42,13 @@ GEM multipart-post (2.1.1) nap (1.1.0) no_proxy_fix (0.1.2) - nokogiri (1.10.9) + nokogiri (1.10.10) mini_portile2 (~> 2.4.0) octokit (4.18.0) faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) open4 (1.3.4) - pmdtester (1.0.0) + pmdtester (1.0.1) differ (~> 0.1) nokogiri (~> 1.8) rufus-scheduler (~> 3.5) @@ -56,7 +56,7 @@ GEM public_suffix (4.0.5) raabro (1.3.1) rchardet (1.8.0) - rouge (3.19.0) + rouge (3.20.0) rufus-scheduler (3.6.0) fugit (~> 1.1, >= 1.1.6) safe_yaml (1.0.5) diff --git a/docs/_plugins/javadoc_tag.rb b/docs/_plugins/javadoc_tag.rb index 56b6e78561..6ca08afdf0 100644 --- a/docs/_plugins/javadoc_tag.rb +++ b/docs/_plugins/javadoc_tag.rb @@ -180,6 +180,9 @@ class JavadocTag < Liquid::Tag if member_suffix && Regexp.new('(\w+|)(' + ARGUMENTS_REGEX.source + ")?") =~ member_suffix suffix = $1 # method or field name + if suffix == '' + suffix = '<init>' + end if opts.show_args? && $2 && !$2.empty? # is method diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 0d181f5c94..710dd82c4a 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -30,6 +30,10 @@ This is a {{ site.pmd.release_type }} release. * core * [#710](https://github.com/pmd/pmd/issues/710): \[core] Review used dependencies * [#2594](https://github.com/pmd/pmd/issues/2594): \[core] Update exec-maven-plugin and align it in all project + * [#2615](https://github.com/pmd/pmd/issues/2615): \[core] PMD/CPD produces invalid XML (insufficient escaping/wrong encoding) +* java-bestpractices + * [#2543](https://github.com/pmd/pmd/issues/2543): \[java] UseCollectionIsEmpty can not detect the case this.foo.size() + * [#2569](https://github.com/pmd/pmd/issues/2569): \[java] LiteralsFirstInComparisons: False negative for methods returning Strings * java-design * [#2174](https://github.com/pmd/pmd/issues/2174): \[java] LawOfDemeter: False positive with 'this' pointer * [#2189](https://github.com/pmd/pmd/issues/2189): \[java] LawOfDemeter: False positive when casting to derived class @@ -50,8 +54,8 @@ This is a {{ site.pmd.release_type }} release. * {% jdoc core::lang.rule.RuleChainVisitor %} and all implementations in language modules * {% jdoc core::lang.rule.AbstractRuleChainVisitor %} -* {% jdoc core::lang.Language#getRuleChainVisitorClass() %} -* {% jdoc core::lang.BaseLanguageModule#(java.lang.String,java.lang.String,java.lang.String,java.lang.Class,java.lang.String...) %} +* {% jdoc !!core::lang.Language#getRuleChainVisitorClass() %} +* {% jdoc !!core::lang.BaseLanguageModule#(java.lang.String,java.lang.String,java.lang.String,java.lang.Class,java.lang.String...) %} ### External Contributions @@ -63,6 +67,8 @@ This is a {{ site.pmd.release_type }} release. * [#2640](https://github.com/pmd/pmd/pull/2640): \[java] NullPointerException in rule ProperCloneImplementation - [Mykhailo Palahuta](https://github.com/Drofff) * [#2641](https://github.com/pmd/pmd/pull/2641): \[java] AvoidThrowingNullPointerException marks all NullPointerException… - [Mykhailo Palahuta](https://github.com/Drofff) * [#2643](https://github.com/pmd/pmd/pull/2643): \[java] AvoidCallingFinalize detects some false positives (2578) - [Mykhailo Palahuta](https://github.com/Drofff) +* [#2651](https://github.com/pmd/pmd/pull/2651): \[java] False negative: LiteralsFirstInComparisons for methods... (2569) - [Mykhailo Palahuta](https://github.com/Drofff) +* [#2652](https://github.com/pmd/pmd/pull/2652): \[java] UseCollectionIsEmpty can not detect the case this.foo.size() - [Mykhailo Palahuta](https://github.com/Drofff) {% endtocmaker %} diff --git a/pmd-core/pom.xml b/pmd-core/pom.xml index 9590252ed0..1cd5be8b2f 100644 --- a/pmd-core/pom.xml +++ b/pmd-core/pom.xml @@ -137,6 +137,10 @@ org.apache.commons commons-lang3 + + org.apache.commons + commons-text + org.ow2.asm asm diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java b/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java index e93c667ac1..066d2fc935 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java @@ -217,8 +217,7 @@ public class PMD { try (TimedOperation to = TimeTracker.startOperation(TimedOperationCategory.REPORTING)) { renderer = configuration.createRenderer(); renderers = Collections.singletonList(renderer); - - renderer.setWriter(IOUtil.createWriter(configuration.getReportFile())); + renderer.setReportFile(configuration.getReportFile()); renderer.start(); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java b/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java index 532d1b6575..694519e344 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java @@ -19,7 +19,6 @@ import net.sourceforge.pmd.lang.LanguageVersionDiscoverer; import net.sourceforge.pmd.renderers.Renderer; import net.sourceforge.pmd.renderers.RendererFactory; import net.sourceforge.pmd.util.ClasspathClassLoader; -import net.sourceforge.pmd.util.IOUtil; /** * This class contains the details for the runtime configuration of PMD. There @@ -406,7 +405,7 @@ public class PMDConfiguration extends AbstractConfiguration { renderer.setUseShortNames(Arrays.asList(inputPaths.split(","))); } if (withReportWriter) { - renderer.setWriter(IOUtil.createWriter(reportFile)); + renderer.setReportFile(reportFile); } return renderer; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java index 33230152d9..5d8ee511d7 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java @@ -18,10 +18,12 @@ import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; +import org.apache.commons.text.StringEscapeUtils; import org.w3c.dom.Document; import org.w3c.dom.Element; import net.sourceforge.pmd.cpd.renderer.CPDRenderer; +import net.sourceforge.pmd.util.StringUtil; /** * @author Philippe T'Seyen - original implementation @@ -76,6 +78,7 @@ public final class XMLRenderer implements Renderer, CPDRenderer { try { TransformerFactory tf = TransformerFactory.newInstance(); Transformer transformer = tf.newTransformer(); + transformer.setOutputProperty(OutputKeys.VERSION, "1.0"); transformer.setOutputProperty(OutputKeys.METHOD, "xml"); transformer.setOutputProperty(OutputKeys.ENCODING, encoding); transformer.setOutputProperty(OutputKeys.INDENT, "yes"); @@ -119,7 +122,9 @@ public final class XMLRenderer implements Renderer, CPDRenderer { mark = iterator.next(); final Element file = doc.createElement("file"); file.setAttribute("line", String.valueOf(mark.getBeginLine())); - file.setAttribute("path", mark.getFilename()); + // only remove invalid characters, escaping is done by the DOM impl. + String filenameXml10 = StringUtil.removedInvalidXml10Characters(mark.getFilename()); + file.setAttribute("path", filenameXml10); file.setAttribute("endline", String.valueOf(mark.getEndLine())); final int beginCol = mark.getBeginColumn(); final int endCol = mark.getEndColumn(); @@ -140,7 +145,7 @@ public final class XMLRenderer implements Renderer, CPDRenderer { // the code snippet has normalized line endings String platformSpecific = codeSnippet.replace("\n", System.lineSeparator()); Element codefragment = doc.createElement("codefragment"); - codefragment.appendChild(doc.createCDATASection(platformSpecific)); + codefragment.appendChild(doc.createCDATASection(StringEscapeUtils.escapeXml10(platformSpecific))); duplication.appendChild(codefragment); } return duplication; diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java index 6279575266..88b69d2202 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java @@ -12,9 +12,11 @@ import java.util.List; import org.apache.commons.io.IOUtils; import net.sourceforge.pmd.PMDConfiguration; +import net.sourceforge.pmd.annotation.Experimental; import net.sourceforge.pmd.cli.PMDParameters; import net.sourceforge.pmd.internal.util.ShortFilenameUtil; import net.sourceforge.pmd.properties.AbstractPropertySource; +import net.sourceforge.pmd.util.IOUtil; /** * Abstract base class for {@link Renderer} implementations. @@ -109,4 +111,16 @@ public abstract class AbstractRenderer extends AbstractPropertySource implements IOUtils.closeQuietly(writer); } } + + /** + * {@inheritDoc} + * + *

This default implementation always uses the system default charset for the writer. + * Overwrite in specific renderers to support other charsets. + */ + @Experimental + @Override + public void setReportFile(String reportFilename) { + this.setWriter(IOUtil.createWriter(reportFilename)); + } } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java index 6f42f25b78..3a2d8f8601 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java @@ -9,6 +9,7 @@ import java.io.Writer; import java.util.List; import net.sourceforge.pmd.Report; +import net.sourceforge.pmd.annotation.Experimental; import net.sourceforge.pmd.properties.PropertyDescriptor; import net.sourceforge.pmd.properties.PropertySource; import net.sourceforge.pmd.util.datasource.DataSource; @@ -163,4 +164,17 @@ public interface Renderer extends PropertySource { void end() throws IOException; void flush() throws IOException; + + /** + * Sets the filename where the report should be written to. If no filename is provided, + * the renderer should write to stdout. + * + *

Implementations must initialize the writer of the renderer. + * + *

See {@link AbstractRenderer#setReportFile(String)} for the default impl. + * + * @param reportFilename the filename (optional). + */ + @Experimental + void setReportFile(String reportFilename); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java index eb71e43749..aba0dd8265 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java @@ -4,10 +4,23 @@ package net.sourceforge.pmd.renderers; +import java.io.File; import java.io.IOException; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.UnsupportedEncodingException; +import java.io.Writer; +import java.nio.file.Files; import java.text.SimpleDateFormat; import java.util.Date; import java.util.Iterator; +import javax.xml.XMLConstants; +import javax.xml.stream.XMLOutputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamWriter; + +import org.apache.commons.io.output.WriterOutputStream; +import org.apache.commons.lang3.StringUtils; import net.sourceforge.pmd.PMD; import net.sourceforge.pmd.PMDVersion; @@ -26,7 +39,13 @@ public class XMLRenderer extends AbstractIncrementingRenderer { // TODO 7.0.0 use PropertyDescriptor or something more specialized public static final StringProperty ENCODING = new StringProperty("encoding", "XML encoding format, defaults to UTF-8.", "UTF-8", 0); - private boolean useUTF8 = false; + + private static final String PMD_REPORT_NS_URI = "http://pmd.sourceforge.net/report/2.0.0"; + private static final String PMD_REPORT_NS_LOCATION = "http://pmd.sourceforge.net/report_2_0_0.xsd"; + private static final String XSI_NS_PREFIX = "xsi"; + + private XMLStreamWriter xmlWriter; + private OutputStream stream; public XMLRenderer() { super(NAME, "XML format."); @@ -46,137 +65,185 @@ public class XMLRenderer extends AbstractIncrementingRenderer { @Override public void start() throws IOException { String encoding = getProperty(ENCODING); - if ("utf-8".equalsIgnoreCase(encoding)) { - useUTF8 = true; - } - StringBuilder buf = new StringBuilder(500); - buf.append("").append(PMD.EOL); - createVersionAttr(buf); - createTimestampAttr(buf); - // FIXME: elapsed time not available until the end of the processing - // buf.append(createTimeElapsedAttr(report)); - buf.append('>').append(PMD.EOL); - writer.write(buf.toString()); + try { + xmlWriter.writeStartDocument(encoding, "1.0"); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.setDefaultNamespace(PMD_REPORT_NS_URI); + xmlWriter.writeStartElement(PMD_REPORT_NS_URI, "pmd"); + xmlWriter.writeDefaultNamespace(PMD_REPORT_NS_URI); + xmlWriter.writeNamespace(XSI_NS_PREFIX, XMLConstants.W3C_XML_SCHEMA_INSTANCE_NS_URI); + xmlWriter.writeAttribute(XSI_NS_PREFIX, XMLConstants.W3C_XML_SCHEMA_INSTANCE_NS_URI, "schemaLocation", + PMD_REPORT_NS_URI + " " + PMD_REPORT_NS_LOCATION); + xmlWriter.writeAttribute("version", PMDVersion.VERSION); + xmlWriter.writeAttribute("timestamp", new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS").format(new Date())); + // FIXME: elapsed time not available until the end of the processing + // xmlWriter.writeAttribute("time_elapsed", ...); + } catch (XMLStreamException e) { + throw new IOException(e); + } } @Override public void renderFileViolations(Iterator violations) throws IOException { - StringBuilder buf = new StringBuilder(500); String filename = null; - // rule violations - while (violations.hasNext()) { - buf.setLength(0); - RuleViolation rv = violations.next(); - String nextFilename = determineFileName(rv.getFilename()); - if (!nextFilename.equals(filename)) { - // New File - if (filename != null) { - // Not first file ? - buf.append("").append(PMD.EOL); + try { + // rule violations + while (violations.hasNext()) { + RuleViolation rv = violations.next(); + String nextFilename = determineFileName(rv.getFilename()); + if (!nextFilename.equals(filename)) { + // New File + if (filename != null) { + // Not first file ? + xmlWriter.writeEndElement(); + } + filename = nextFilename; + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("file"); + xmlWriter.writeAttribute("name", filename); + xmlWriter.writeCharacters(PMD.EOL); } - filename = nextFilename; - buf.append("").append(PMD.EOL); + + xmlWriter.writeStartElement("violation"); + xmlWriter.writeAttribute("beginline", String.valueOf(rv.getBeginLine())); + xmlWriter.writeAttribute("endline", String.valueOf(rv.getEndLine())); + xmlWriter.writeAttribute("begincolumn", String.valueOf(rv.getBeginColumn())); + xmlWriter.writeAttribute("endcolumn", String.valueOf(rv.getEndColumn())); + xmlWriter.writeAttribute("rule", rv.getRule().getName()); + xmlWriter.writeAttribute("ruleset", rv.getRule().getRuleSetName()); + maybeAdd("package", rv.getPackageName()); + maybeAdd("class", rv.getClassName()); + maybeAdd("method", rv.getMethodName()); + maybeAdd("variable", rv.getVariableName()); + maybeAdd("externalInfoUrl", rv.getRule().getExternalInfoUrl()); + xmlWriter.writeAttribute("priority", String.valueOf(rv.getRule().getPriority().getPriority())); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeCharacters(StringUtil.removedInvalidXml10Characters(rv.getDescription())); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); + xmlWriter.writeCharacters(PMD.EOL); } - - buf.append("").append(PMD.EOL); - StringUtil.appendXmlEscaped(buf, rv.getDescription(), useUTF8); - - buf.append(PMD.EOL); - buf.append(""); - buf.append(PMD.EOL); - writer.write(buf.toString()); - } - if (filename != null) { // Not first file ? - writer.write(""); - writer.write(PMD.EOL); + if (filename != null) { // Not first file ? + xmlWriter.writeEndElement(); + } + } catch (XMLStreamException e) { + throw new IOException(e); } } @Override public void end() throws IOException { - StringBuilder buf = new StringBuilder(500); - // errors - for (Report.ProcessingError pe : errors) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - buf.append("").append(PMD.EOL); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); - } - - // suppressed violations - if (showSuppressedViolations) { - for (Report.SuppressedViolation s : suppressed) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); + try { + // errors + for (Report.ProcessingError pe : errors) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("error"); + xmlWriter.writeAttribute("filename", determineFileName(pe.getFile())); + xmlWriter.writeAttribute("msg", pe.getMsg()); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeCData(pe.getDetail()); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); } - } - // config errors - for (final Report.ConfigurationError ce : configErrors) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); - } + // suppressed violations + if (showSuppressedViolations) { + for (Report.SuppressedViolation s : suppressed) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("suppressedviolation"); + xmlWriter.writeAttribute("filename", determineFileName(s.getRuleViolation().getFilename())); + xmlWriter.writeAttribute("suppressiontype", s.suppressedByNOPMD() ? "nopmd" : "annotation"); + xmlWriter.writeAttribute("msg", s.getRuleViolation().getDescription()); + xmlWriter.writeAttribute("usermsg", s.getUserMessage() == null ? "" : s.getUserMessage()); + xmlWriter.writeEndElement(); + } + } - writer.write("" + PMD.EOL); + // config errors + for (final Report.ConfigurationError ce : configErrors) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEmptyElement("configerror"); + xmlWriter.writeAttribute("rule", ce.rule().getName()); + xmlWriter.writeAttribute("msg", ce.issue()); + } + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); // + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.flush(); + } catch (XMLStreamException e) { + throw new IOException(e); + } } - private void maybeAdd(String attr, String value, StringBuilder buf) { + private void maybeAdd(String attr, String value) throws XMLStreamException { if (value != null && value.length() > 0) { - buf.append(' ').append(attr).append("=\""); - StringUtil.appendXmlEscaped(buf, value, useUTF8); - buf.append('"'); + xmlWriter.writeAttribute(attr, value); } } - private void createVersionAttr(StringBuilder buffer) { - buffer.append("In contrast to {@link Charset#defaultCharset()}, the result is not cached, + * so that in unit tests, the charset can be changed. + * @return + */ + private static Charset getDefaultCharset() { + String csn = AccessController.doPrivileged(new PrivilegedAction() { + @Override + public String run() { + return System.getProperty("file.encoding"); + } + }); + try { + return Charset.forName(csn); + } catch (UnsupportedCharsetException e) { + return StandardCharsets.UTF_8; + } + } + + /** + * Creates a writer that writes to the given file or to stdout. + * + *

Warning: This writer always uses the system default charset. + * + * @param reportFile the file name (optional) + * @return the writer, never null + */ public static Writer createWriter(String reportFile) { try { return StringUtils.isBlank(reportFile) ? createWriter() - : Files.newBufferedWriter(new File(reportFile).toPath(), Charset.defaultCharset()); + : Files.newBufferedWriter(new File(reportFile).toPath(), getDefaultCharset()); } catch (IOException e) { throw new IllegalArgumentException(e); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java index fcb8aaaab8..c4f5fa1422 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java @@ -9,8 +9,11 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.text.StringEscapeUtils; import net.sourceforge.pmd.annotation.InternalApi; @@ -27,6 +30,12 @@ public final class StringUtil { private static final String[] EMPTY_STRINGS = new String[0]; + private static final Pattern XML_10_INVALID_CHARS = Pattern.compile( + "\\x00|\\x01|\\x02|\\x03|\\x04|\\x05|\\x06|\\x07|\\x08|" + + "\\x0b|\\x0c|\\x0e|\\x0f|" + + "\\x10|\\x11|\\x12|\\x13|\\x14|\\x15|\\x16|\\x17|\\x18|" + + "\\x19|\\x1a|\\x1b|\\x1c|\\x1d|\\x1e|\\x1f"); + private StringUtil() { } @@ -211,8 +220,9 @@ public final class StringUtil { * @param src * @param supportUTF8 override the default setting, whether special characters should be replaced with entities ( * false) or should be included as is ( true). - * + * @deprecated for removal. Use {@link StringEscapeUtils#escapeXml10(String)} instead. */ + @Deprecated public static void appendXmlEscaped(StringBuilder buf, String src, boolean supportUTF8) { char c; int i = 0; @@ -245,6 +255,20 @@ public final class StringUtil { } } + /** + * Remove characters, that are not allowed in XML 1.0 documents. + * + *

Allowed characters are: + *

+ * Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] + * // any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. + *
+ * (see Extensible Markup Language (XML) 1.0 (Fifth Edition)). + */ + public static String removedInvalidXml10Characters(String text) { + Matcher matcher = XML_10_INVALID_CHARS.matcher(text); + return matcher.replaceAll(""); + } /** * Replace some whitespace characters so they are visually apparent. diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java index 2e5f2c5e16..224c2b4064 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java @@ -1,4 +1,4 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ @@ -9,11 +9,16 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; import java.io.StringWriter; +import java.nio.charset.Charset; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.Iterator; import java.util.Map; +import org.apache.commons.io.IOUtils; import org.junit.Test; import net.sourceforge.pmd.lang.ast.DummyNode; @@ -196,4 +201,21 @@ public class ReportTest implements ThreadSafeReportListener { renderer.end(); return writer.toString(); } + + public static String renderTempFile(Renderer renderer, Report report, Charset expectedCharset) throws IOException { + Path tempFile = Files.createTempFile("pmd-report-test", null); + String absolutePath = tempFile.toAbsolutePath().toString(); + + renderer.setReportFile(absolutePath); + renderer.start(); + renderer.renderFileReport(report); + renderer.end(); + renderer.flush(); + + try (FileInputStream input = new FileInputStream(absolutePath)) { + return IOUtils.toString(input, expectedCharset); + } finally { + Files.delete(tempFile); + } + } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java index 22eca255a7..d5340d2c67 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.cpd; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -30,6 +31,9 @@ import net.sourceforge.pmd.cpd.renderer.CPDRenderer; public class XMLRendererTest { private static final String ENCODING = (String) System.getProperties().get("file.encoding"); + private static final String FORM_FEED = "\u000C"; // this character is invalid in XML 1.0 documents + private static final String FORM_FEED_ENTITY = " "; // this is also not allowed in XML 1.0 documents + @Test public void testWithNoDuplication() throws IOException { @@ -185,8 +189,8 @@ public class XMLRendererTest { CPDRenderer renderer = new XMLRenderer(); List list = new ArrayList<>(); final String espaceChar = "<"; - Mark mark1 = createMark("public", "/var/F" + '<' + "oo.java", 48, 6, "code fragment"); - Mark mark2 = createMark("void", "/var/F list = new ArrayList<>(); + Mark mark1 = createMark("public", "file1", 1, 2, codefragment); + Mark mark2 = createMark("public", "file2", 5, 2, codefragment); + Match match1 = new Match(75, mark1, mark2); + list.add(match1); + + StringWriter sw = new StringWriter(); + renderer.render(list.iterator(), sw); + String report = sw.toString(); + assertFalse(report.contains(FORM_FEED)); + assertFalse(report.contains(FORM_FEED_ENTITY)); } private Mark createMark(String image, String tokenSrcID, int beginLine, int lineCount, String code) { @@ -214,8 +237,4 @@ public class XMLRendererTest { result.setSourceCode(new SourceCode(new SourceCode.StringCodeLoader(code))); return result; } - - public static junit.framework.Test suite() { - return new junit.framework.JUnit4TestAdapter(XMLRendererTest.class); - } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java index 675574e4ac..390d3a812e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java @@ -6,10 +6,14 @@ package net.sourceforge.pmd.renderers; import java.io.File; import java.io.StringReader; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import javax.xml.parsers.DocumentBuilderFactory; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.w3c.dom.Document; import org.w3c.dom.NodeList; import org.xml.sax.InputSource; @@ -28,6 +32,8 @@ import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.ParametricRuleViolation; public class XMLRendererTest extends AbstractRendererTest { + @Rule // Restores system properties after test + public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties(); @Override public Renderer getRenderer() { @@ -90,12 +96,13 @@ public class XMLRendererTest extends AbstractRendererTest { return new ParametricRuleViolation(new FooRule(), ctx, node, description); } - private void verifyXmlEscaping(Renderer renderer, String shouldContain) throws Exception { + private void verifyXmlEscaping(Renderer renderer, String shouldContain, Charset charset) throws Exception { + renderer.setProperty(XMLRenderer.ENCODING, charset.name()); Report report = new Report(); String surrogatePair = "\ud801\udc1c"; - String msg = "The String literal \"Tokenizer " + surrogatePair + "\" appears..."; + String msg = "The String 'literal' \"TokénizĀr " + surrogatePair + "\" appears..."; report.addRuleViolation(createRuleViolation(msg)); - String actual = ReportTest.render(renderer, report); + String actual = ReportTest.renderTempFile(renderer, report, charset); Assert.assertTrue(actual.contains(shouldContain)); Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder() .parse(new InputSource(new StringReader(actual))); @@ -107,22 +114,44 @@ public class XMLRendererTest extends AbstractRendererTest { @Test public void testXMLEscapingWithUTF8() throws Exception { Renderer renderer = getRenderer(); - renderer.setProperty(XMLRenderer.ENCODING, "UTF-8"); - verifyXmlEscaping(renderer, "\ud801\udc1c"); + verifyXmlEscaping(renderer, "\ud801\udc1c", StandardCharsets.UTF_8); } @Test public void testXMLEscapingWithoutUTF8() throws Exception { Renderer renderer = getRenderer(); - renderer.setProperty(XMLRenderer.ENCODING, "ISO-8859-1"); - verifyXmlEscaping(renderer, "𐐜"); + verifyXmlEscaping(renderer, "𐐜", StandardCharsets.ISO_8859_1); } public String getHeader() { return "" + PMD.EOL - + "" + PMD.EOL; + + "" + PMD.EOL; + } + + @Test + public void testCorrectCharset() throws Exception { + System.setProperty("file.encoding", StandardCharsets.ISO_8859_1.name()); + + Renderer renderer = getRenderer(); + + Report report = new Report(); + String formFeed = "\u000C"; + // é = U+00E9 : can be represented in ISO-8859-1 as is + // Ā = U+0100 : cannot be represented in ISO-8859-1 -> would be a unmappable character, needs to be escaped + String specialChars = "éĀ"; + String originalChars = formFeed + specialChars; // u000C should be removed, é should be encoded correctly as UTF-8 + String msg = "The String literal \"" + originalChars + "\" appears..."; + report.addRuleViolation(createRuleViolation(msg)); + String actual = ReportTest.renderTempFile(renderer, report, StandardCharsets.UTF_8); + Assert.assertTrue(actual.contains(specialChars)); + Assert.assertFalse(actual.contains(formFeed)); + Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder() + .parse(new InputSource(new StringReader(actual))); + NodeList violations = doc.getElementsByTagName("violation"); + Assert.assertEquals(1, violations.getLength()); + Assert.assertEquals(msg.replaceAll(formFeed, ""), violations.item(0).getTextContent().trim()); } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java index 21df9d406e..387928c89e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java @@ -36,14 +36,6 @@ public class StringUtilTest { assertEquals("replaceString didn't work with a char", "f", StringUtil.replaceString("foo", 'o', null)); } - /** - * Usually you would set the system property - * "net.sourceforge.pmd.supportUTF8" to either "no" or "yes", to switch UTF8 - * support. - * - * e.g. - * System.setProperty("net.sourceforge.pmd.supportUTF8","yes"); - */ @Test public void testUTF8NotSupported() { StringBuilder sb = new StringBuilder(); @@ -68,8 +60,4 @@ public class StringUtilTest { StringUtil.appendXmlEscaped(sb, test, true); assertEquals("é", sb.toString()); } - - public static junit.framework.Test suite() { - return new junit.framework.JUnit4TestAdapter(StringUtilTest.class); - } } diff --git a/pmd-doc/pom.xml b/pmd-doc/pom.xml index 318dfa95b3..631bc6c829 100644 --- a/pmd-doc/pom.xml +++ b/pmd-doc/pom.xml @@ -94,7 +94,6 @@ org.apache.commons commons-text - 1.6 org.yaml diff --git a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java index fbb78a6898..75c56f7f49 100644 --- a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java +++ b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java @@ -30,6 +30,11 @@ public final class GenerateRuleDocsCmd { } public static void main(String[] args) throws RuleSetNotFoundException { + if (args.length != 1) { + System.err.println("One argument is required: The base directory of the module pmd-doc."); + System.exit(1); + } + long start = System.currentTimeMillis(); Path output = FileSystems.getDefault().getPath(args[0]).resolve("..").toAbsolutePath().normalize(); System.out.println("Generating docs into " + output); diff --git a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java index 818ad3e7c8..f62400b1a9 100644 --- a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java +++ b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java @@ -75,6 +75,12 @@ public class RuleDocGenerator { private final Path root; private final FileWriter writer; + /** Caches rule class name to java source file mapping. */ + private final Map allRules = new HashMap<>(); + /** Caches ruleset to ruleset xml file mapping. */ + private final Map allRulesets = new HashMap<>(); + + public RuleDocGenerator(FileWriter writer, Path root) { this.writer = Objects.requireNonNull(writer, "A file writer must be provided"); this.root = Objects.requireNonNull(root, "Root directory must be provided"); @@ -91,6 +97,7 @@ public class RuleDocGenerator { try { sortedRulesets = sortRulesets(registeredRulesets); sortedAdditionalRulesets = sortRulesets(resolveAdditionalRulesets(additionalRulesets)); + determineRuleClassSourceFiles(sortedRulesets); generateLanguageIndex(sortedRulesets, sortedAdditionalRulesets); generateRuleSetIndex(sortedRulesets); @@ -345,7 +352,7 @@ public class RuleDocGenerator { String permalink = RULESET_INDEX_PERMALINK_PATTERN .replace("${language.tersename}", languageTersename) .replace("${ruleset.name}", rulesetFilename); - String ruleSetSourceFilepath = "../" + getRuleSetSourceFilepath(ruleset); + String ruleSetSourceFilepath = "../" + allRulesets.get(ruleset.getFileName()); List lines = new LinkedList<>(); lines.add("---"); @@ -415,7 +422,7 @@ public class RuleDocGenerator { } else { lines.add("**This rule is defined by the following Java class:** " + "[" + rule.getRuleClass() + "](" - + GITHUB_SOURCE_LINK + getRuleClassSourceFilepath(rule.getRuleClass()) + + GITHUB_SOURCE_LINK + allRules.get(rule.getRuleClass()) + ")"); } lines.add(""); @@ -607,63 +614,74 @@ public class RuleDocGenerator { } /** - * Searches for the source file of the given ruleset. This provides the information - * for the "editme" link. + * Walks through the root directory once to get all rule source file path names and ruleset names. + * This provides the information for the "editme" links. * - * @param ruleset the ruleset to search for. - * @return - * @throws IOException + * @param sortedRulesets all the rulesets and rules */ - private String getRuleSetSourceFilepath(RuleSet ruleset) throws IOException { - final String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName())); - final List foundPathResult = new LinkedList<>(); - - Files.walkFileTree(root, new SimpleFileVisitor() { - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - String path = file.toString(); - if (path.contains("src") && path.endsWith(rulesetFilename)) { - foundPathResult.add(file); - return FileVisitResult.TERMINATE; + private void determineRuleClassSourceFiles(Map> sortedRulesets) { + // first collect all the classes, we need to resolve and the rulesets + // this also provides a default fallback path, which is used in unit tests. + // if the actual file is found during walkFileTree, then the default fallback path + // is replaced by a correct path. + for (List rulesets : sortedRulesets.values()) { + for (RuleSet ruleset : rulesets) { + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName()), true); + allRulesets.put(ruleset.getFileName(), rulesetFilename); + for (Rule rule : ruleset.getRules()) { + String ruleClass = rule.getRuleClass(); + String relativeSourceFilename = ruleClass.replaceAll("\\.", Matcher.quoteReplacement(File.separator)) + + ".java"; + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRules.put(ruleClass, FilenameUtils.normalize(relativeSourceFilename, true)); } - return super.visitFile(file, attrs); } - }); - - if (!foundPathResult.isEmpty()) { - Path foundPath = foundPathResult.get(0); - foundPath = root.relativize(foundPath); - // Note: the path is normalized to unix path separators, so that the editme link - // uses forward slashes - return FilenameUtils.normalize(foundPath.toString(), true); } - return StringUtils.chomp(ruleset.getFileName()); - } + // then go and search the actual files + try { + Files.walkFileTree(root, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { + String path = file.toString(); - private String getRuleClassSourceFilepath(String ruleClass) throws IOException { - final String relativeSourceFilename = ruleClass.replaceAll("\\.", Matcher.quoteReplacement(File.separator)) - + ".java"; - final List foundPathResult = new LinkedList<>(); + if (path.contains("src")) { + String foundRuleClass = null; + for (Map.Entry entry : allRules.entrySet()) { + if (path.endsWith(entry.getValue())) { + foundRuleClass = entry.getKey(); + break; + } + } + if (foundRuleClass != null) { + Path foundPath = root.relativize(file); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRules.put(foundRuleClass, FilenameUtils.normalize(foundPath.toString(), true)); + } - Files.walkFileTree(root, new SimpleFileVisitor() { - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - String path = file.toString(); - if (path.contains("src") && path.endsWith(relativeSourceFilename)) { - foundPathResult.add(file); - return FileVisitResult.TERMINATE; + String foundRuleset = null; + for (Map.Entry entry : allRulesets.entrySet()) { + if (path.endsWith(entry.getValue())) { + foundRuleset = entry.getKey(); + break; + } + } + if (foundRuleset != null) { + Path foundPath = root.relativize(file); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRulesets.put(foundRuleset, FilenameUtils.normalize(foundPath.toString(), true)); + } + } + return FileVisitResult.CONTINUE; } - return super.visitFile(file, attrs); - } - }); - - if (!foundPathResult.isEmpty()) { - Path foundPath = foundPathResult.get(0); - foundPath = root.relativize(foundPath); - return FilenameUtils.normalize(foundPath.toString(), true); + }); + } catch (IOException e) { + throw new RuntimeException(e); } - - return FilenameUtils.normalize(relativeSourceFilename, true); } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java index 667240f5d8..3419dd8d65 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java @@ -4,7 +4,8 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices; -import net.sourceforge.pmd.lang.ast.Node; +import java.util.List; + import net.sourceforge.pmd.lang.java.ast.ASTArgumentList; import net.sourceforge.pmd.lang.java.ast.ASTArguments; import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression; @@ -29,63 +30,83 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { } @Override - public Object visit(ASTPrimaryExpression node, Object data) { - ASTPrimaryPrefix primaryPrefix = node.getFirstChildOfType(ASTPrimaryPrefix.class); - ASTPrimarySuffix primarySuffix = node.getFirstChildOfType(ASTPrimarySuffix.class); - if (primaryPrefix != null && primarySuffix != null) { - ASTName name = primaryPrefix.getFirstChildOfType(ASTName.class); - if (name == null || isIrrelevantImage(name.getImage())) { - return data; - } - if (!isSingleStringLiteralArgument(primarySuffix)) { - return data; - } - if (isWithinNullComparison(node)) { - return data; - } - addViolation(data, node); + public Object visit(ASTPrimaryExpression expression, Object data) { + if (violatesLiteralsFirstInComparisonsRule(expression)) { + addViolation(data, expression); } - return node; + return data; } - private boolean isIrrelevantImage(String image) { + private boolean violatesLiteralsFirstInComparisonsRule(ASTPrimaryExpression expression) { + return !hasStringLiteralFirst(expression) && isNullableComparisonWithStringLiteral(expression); + } + + private boolean hasStringLiteralFirst(ASTPrimaryExpression expression) { + ASTPrimaryPrefix primaryPrefix = expression.getFirstChildOfType(ASTPrimaryPrefix.class); + ASTLiteral firstLiteral = primaryPrefix.getFirstDescendantOfType(ASTLiteral.class); + return firstLiteral != null && firstLiteral.isStringLiteral(); + } + + private boolean isNullableComparisonWithStringLiteral(ASTPrimaryExpression expression) { + String opName = getOperationName(expression); + ASTPrimarySuffix argsSuffix = getSuffixOfArguments(expression); + return opName != null && argsSuffix != null && isStringLiteralComparison(opName, argsSuffix) + && isNotWithinNullComparison(expression); + } + + private String getOperationName(ASTPrimaryExpression primaryExpression) { + return isMethodsChain(primaryExpression) ? getOperationNameBySuffix(primaryExpression) + : getOperationNameByPrefix(primaryExpression); + } + + private boolean isMethodsChain(ASTPrimaryExpression primaryExpression) { + return primaryExpression.getNumChildren() > 2; + } + + private String getOperationNameBySuffix(ASTPrimaryExpression primaryExpression) { + ASTPrimarySuffix opAsSuffix = getPrimarySuffixAtIndexFromEnd(primaryExpression, 1); + if (opAsSuffix != null) { + String opName = opAsSuffix.getImage(); // name of pattern "operation" + return "." + opName; + } + return null; + } + + private String getOperationNameByPrefix(ASTPrimaryExpression primaryExpression) { + ASTPrimaryPrefix opAsPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + if (opAsPrefix != null) { + ASTName opName = opAsPrefix.getFirstChildOfType(ASTName.class); // name of pattern "*.operation" + return opName != null ? opName.getImage() : null; + } + return null; + } + + private ASTPrimarySuffix getSuffixOfArguments(ASTPrimaryExpression primaryExpression) { + return getPrimarySuffixAtIndexFromEnd(primaryExpression, 0); + } + + private ASTPrimarySuffix getPrimarySuffixAtIndexFromEnd(ASTPrimaryExpression primaryExpression, int indexFromEnd) { + List primarySuffixes = primaryExpression.findChildrenOfType(ASTPrimarySuffix.class); + if (!primarySuffixes.isEmpty()) { + int suffixIndex = primarySuffixes.size() - 1 - indexFromEnd; + return primarySuffixes.get(suffixIndex); + } + return null; + } + + private boolean isStringLiteralComparison(String opName, ASTPrimarySuffix argsSuffix) { + return isComparisonOperation(opName) && isSingleStringLiteralArgument(argsSuffix); + } + + private boolean isComparisonOperation(String op) { for (String comparisonOp : COMPARISON_OPS) { - if (image.endsWith(comparisonOp)) { - return false; - } - } - return true; - } - - private boolean isWithinNullComparison(ASTPrimaryExpression node) { - for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) { - if (isComparisonWithNull(parentExpr, "==", ASTConditionalOrExpression.class) - || isComparisonWithNull(parentExpr, "!=", ASTConditionalAndExpression.class)) { + if (op.endsWith(comparisonOp)) { return true; } } return false; } - /* - * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral - * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral - */ - private boolean isComparisonWithNull(ASTExpression parentExpr, String equalOperator, Class condition) { - Node condExpr = null; - ASTEqualityExpression eqExpr = null; - if (parentExpr != null) { - condExpr = parentExpr.getFirstChildOfType(condition); - } - if (condExpr != null) { - eqExpr = condExpr.getFirstDescendantOfType(ASTEqualityExpression.class); - } - if (eqExpr != null) { - return eqExpr.hasImageEqualTo(equalOperator) && eqExpr.hasDescendantOfType(ASTNullLiteral.class); - } - return false; - } - /* * This corresponds to the following XPath expression: * (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral= true()]) @@ -93,35 +114,77 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { * ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 ) */ private boolean isSingleStringLiteralArgument(ASTPrimarySuffix primarySuffix) { - if (!primarySuffix.isArguments() || primarySuffix.getArgumentCount() != 1) { + return isSingleArgumentSuffix(primarySuffix) && isStringLiteralFirstArgumentOfSuffix(primarySuffix); + } + + private boolean isSingleArgumentSuffix(ASTPrimarySuffix primarySuffix) { + return primarySuffix.getArgumentCount() == 1; + } + + private boolean isStringLiteralFirstArgumentOfSuffix(ASTPrimarySuffix primarySuffix) { + try { + JavaNode firstArg = getFirstArgument(primarySuffix); + return isStringLiteral(firstArg); + } catch (NullPointerException e) { return false; } - Node node = primarySuffix; - node = node.getFirstChildOfType(ASTArguments.class); - if (node != null) { - node = node.getFirstChildOfType(ASTArgumentList.class); - if (node.getNumChildren() != 1) { - return false; - } - } - if (node != null) { - node = node.getFirstChildOfType(ASTExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryPrefix.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTLiteral.class); - } - if (node != null) { + } + + private JavaNode getFirstArgument(ASTPrimarySuffix primarySuffix) { + ASTArguments arguments = primarySuffix.getFirstChildOfType(ASTArguments.class); + ASTArgumentList argumentList = arguments.getFirstChildOfType(ASTArgumentList.class); + ASTExpression expression = argumentList.getFirstChildOfType(ASTExpression.class); + ASTPrimaryExpression primaryExpression = expression.getFirstChildOfType(ASTPrimaryExpression.class); + ASTPrimaryPrefix primaryPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + return primaryPrefix.getFirstChildOfType(ASTLiteral.class); + } + + private boolean isStringLiteral(JavaNode node) { + if (node instanceof ASTLiteral) { ASTLiteral literal = (ASTLiteral) node; - if (literal.isStringLiteral()) { + return literal.isStringLiteral(); + } + return false; + } + + private boolean isNotWithinNullComparison(ASTPrimaryExpression node) { + return !isWithinNullComparison(node); + } + + /* + * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral + * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral + */ + private boolean isWithinNullComparison(ASTPrimaryExpression node) { + for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) { + if (isNullComparison(parentExpr)) { return true; } } return false; } + + private boolean isNullComparison(ASTExpression expression) { + return isAndNotNullComparison(expression) || isOrNullComparison(expression); + } + + private boolean isAndNotNullComparison(ASTExpression expression) { + ASTConditionalAndExpression andExpression = expression + .getFirstChildOfType(ASTConditionalAndExpression.class); + return andExpression != null && hasEqualityExpressionWithNullLiteral(andExpression, "!="); + } + + private boolean isOrNullComparison(ASTExpression expression) { + ASTConditionalOrExpression orExpression = expression + .getFirstChildOfType(ASTConditionalOrExpression.class); + return orExpression != null && hasEqualityExpressionWithNullLiteral(orExpression, "=="); + } + + private boolean hasEqualityExpressionWithNullLiteral(JavaNode node, String equalityOp) { + ASTEqualityExpression equalityExpression = node.getFirstDescendantOfType(ASTEqualityExpression.class); + if (equalityExpression != null && equalityExpression.hasImageEqualTo(equalityOp)) { + return equalityExpression.hasDescendantOfType(ASTNullLiteral.class); + } + return false; + } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java index ea55ce94c4..552d1116a7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java @@ -4,18 +4,23 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices; -import java.util.Arrays; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; + import java.util.HashMap; import java.util.List; import java.util.Map; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.ast.ASTResultType; +import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.rule.AbstractInefficientZeroCheck; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence; @@ -45,43 +50,78 @@ public class UseCollectionIsEmptyRule extends AbstractInefficientZeroCheck { */ @Override public boolean isTargetMethod(JavaNameOccurrence occ) { - if (occ.getNameForWhichThisIsAQualifier() != null) { - if (occ.getLocation().getImage().endsWith(".size")) { - return true; - } - } - return false; + return occ.getNameForWhichThisIsAQualifier() != null + && occ.getLocation().getImage().endsWith(".size"); } @Override public Map> getComparisonTargets() { + List zeroAndOne = asList("0", "1"); + List zero = singletonList("0"); Map> rules = new HashMap<>(); - rules.put("<", Arrays.asList("0", "1")); - rules.put(">", Arrays.asList("0")); - rules.put("==", Arrays.asList("0")); - rules.put("!=", Arrays.asList("0")); - rules.put(">=", Arrays.asList("0", "1")); - rules.put("<=", Arrays.asList("0")); + rules.put("<", zeroAndOne); + rules.put(">", zero); + rules.put("==", zero); + rules.put("!=", zero); + rules.put(">=", zeroAndOne); + rules.put("<=", zero); return rules; } @Override public Object visit(ASTPrimarySuffix node, Object data) { - if (node.getImage() != null && node.getImage().endsWith("size")) { - - ASTClassOrInterfaceType type = getTypeOfPrimaryPrefix(node); - if (type == null) { - type = getTypeOfMethodCall(node); - } - - if (type != null && CollectionUtil.isCollectionType(type.getType(), true)) { - Node expr = node.getParent().getParent(); - checkNodeAndReport(data, node, expr); - } + if (isSizeMethodCall(node) && isCalledOnCollection(node)) { + Node expr = node.getParent().getParent(); + checkNodeAndReport(data, node, expr); } return data; } + private boolean isSizeMethodCall(ASTPrimarySuffix primarySuffix) { + String calledMethodName = primarySuffix.getImage(); + return calledMethodName != null && calledMethodName.endsWith("size"); + } + + private boolean isCalledOnCollection(ASTPrimarySuffix primarySuffix) { + ASTClassOrInterfaceType calledOnType = getTypeOfVariable(primarySuffix); + if (calledOnType == null) { + calledOnType = getTypeOfMethodCall(primarySuffix); + } + return calledOnType != null + && CollectionUtil.isCollectionType(calledOnType.getType(), true); + } + + private ASTClassOrInterfaceType getTypeOfVariable(ASTPrimarySuffix primarySuffix) { + ASTPrimaryExpression primaryExpression = primarySuffix.getFirstParentOfType(ASTPrimaryExpression.class); + ASTPrimaryPrefix varPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + if (prefixWithNoModifiers(varPrefix)) { + return varPrefix.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + } + String varName = getVariableNameBySuffix(primaryExpression); + return varName != null ? getTypeOfVariableByName(varName, primaryExpression) : null; + } + + private boolean prefixWithNoModifiers(ASTPrimaryPrefix primaryPrefix) { + return !primaryPrefix.usesSuperModifier() && !primaryPrefix.usesThisModifier(); + } + + private String getVariableNameBySuffix(ASTPrimaryExpression primaryExpression) { + ASTPrimarySuffix varSuffix = primaryExpression + .getFirstChildOfType(ASTPrimarySuffix.class); + return varSuffix.getImage(); + } + + private ASTClassOrInterfaceType getTypeOfVariableByName(String varName, ASTPrimaryExpression expr) { + ASTClassOrInterfaceBody classBody = expr.getFirstParentOfType(ASTClassOrInterfaceBody.class); + List varDeclarators = classBody.findDescendantsOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator varDeclarator : varDeclarators) { + if (varDeclarator.getName().equals(varName)) { + return varDeclarator.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + } + } + return null; + } + private ASTClassOrInterfaceType getTypeOfMethodCall(ASTPrimarySuffix node) { ASTClassOrInterfaceType type = null; ASTName methodName = node.getParent().getFirstChildOfType(ASTPrimaryPrefix.class) @@ -100,9 +140,4 @@ public class UseCollectionIsEmptyRule extends AbstractInefficientZeroCheck { } return type; } - - private ASTClassOrInterfaceType getTypeOfPrimaryPrefix(ASTPrimarySuffix node) { - return node.getParent().getFirstChildOfType(ASTPrimaryPrefix.class) - .getFirstDescendantOfType(ASTClassOrInterfaceType.class); - } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java index 2857e65958..82802e409c 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java @@ -7,10 +7,9 @@ package net.sourceforge.pmd.ant; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import java.lang.reflect.Field; +import java.io.UnsupportedEncodingException; import java.nio.charset.Charset; import java.util.Locale; -import java.util.Objects; import org.apache.commons.io.FileUtils; import org.junit.Rule; @@ -111,17 +110,8 @@ public class PMDTaskTest extends AbstractAntTestHelper { } }; - // See http://stackoverflow.com/questions/361975/setting-the-default-java-character-encoding and http://stackoverflow.com/a/14987992/1169968 private static void setDefaultCharset(String charsetName) { - try { - System.setProperty("file.encoding", charsetName); - Field charset = Charset.class.getDeclaredField("defaultCharset"); - charset.setAccessible(true); - charset.set(null, null); - Objects.requireNonNull(Charset.defaultCharset()); - } catch (Exception e) { - throw new RuntimeException(e); - } + System.setProperty("file.encoding", charsetName); } @Rule @@ -149,14 +139,28 @@ public class PMDTaskTest extends AbstractAntTestHelper { assertTrue(report.contains("unusedVariableWithÜmlaut")); } + private static String convert(String report) { + // reinterpret output as cp1252 - ant BuildFileRule can only unicode + StringBuilder sb = new StringBuilder(report.length()); + for (int i = 0; i < report.length(); i++) { + char c = report.charAt(i); + if (c > 0x7f) { + sb.append((char) (c & 0xff)); + } else { + sb.append(c); + } + } + return sb.toString(); + } + @Test - public void testFormatterEncodingWithXMLConsole() { + public void testFormatterEncodingWithXMLConsole() throws UnsupportedEncodingException { setDefaultCharset("cp1252"); executeTarget("testFormatterEncodingWithXMLConsole"); - String report = buildRule.getOutput(); + String report = convert(buildRule.getOutput()); assertTrue(report.startsWith("")); - assertTrue(report.contains("unusedVariableWithÜmlaut")); + assertTrue(report.contains("unusedVariableWithÜmlaut")); } @Test diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java index 6150151d61..dad635c5d5 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java @@ -77,7 +77,7 @@ public class CPDCommandLineInterfaceTest extends BaseCPDCLITest { String out = getOutput(); Assert.assertTrue(out.startsWith("")); - Assert.assertTrue(Pattern.compile("System\\.out\\.println\\([ij] \\+ \"ä\"\\);").matcher(out).find()); + Assert.assertTrue(Pattern.compile("System\\.out\\.println\\([ij] \\+ "ä"\\);").matcher(out).find()); Assert.assertEquals(4, Integer.parseInt(System.getProperty(CPDCommandLineInterface.STATUS_CODE_PROPERTY))); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index 024ff927ab..f76ba2658f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml @@ -272,6 +272,49 @@ public class Foo { boolean bar(String x) { return contentEquals("2"); } +} + ]]> + + + + bad, testing false negative at the end of a chain + 1 + + + + + ok, should be ignored in case both operands are string literals + 0 + + + + + Equals on method result with String argument + 1 + 6 + diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml index 664fdaa1c9..d4f085f0df 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml @@ -7,6 +7,7 @@ fail, == 0 1 + 3 fail, != 0 1 + 3 fail, != 0 1 + 3 fail, 0 == 1 + 3 fail, > 0 1 + 3 #1214 UseCollectionIsEmpty misses some usage 5 + 25,28,31,34,37 + + + + #2543 false negative on this.collection.size + 1 + 10 + list = new ArrayList<>(); + private Bar notACollection = new Bar(); + + public void bar() { + if (this.list.size() == 0) { + throw new RuntimeException("Empty list"); + } + if (notACollection.size() == 0) { } + if (this.notACollection.size() == 0) { } + } + + public int size() { + return 0; + } } ]]> diff --git a/pom.xml b/pom.xml index 4e0026061d..c4a5f7358d 100644 --- a/pom.xml +++ b/pom.xml @@ -696,6 +696,11 @@ commons-lang3 3.8.1 + + org.apache.commons + commons-text + 1.6 + org.slf4j slf4j-api