Fix Gcc parser when there are multiple locations involved

uhafner · uhafner · commit 6f77973536d9 · 2026-04-20T21:22:25.000+02:00
When multiple locations are used, then also the primary location must be provided as a location. Fixes #1488
diff --git a/src/main/java/edu/hm/hafner/analysis/IssueBuilder.java b/src/main/java/edu/hm/hafner/analysis/IssueBuilder.java
@@ -491,6 +491,19 @@ public IssueBuilder guessSeverity(@CheckForNull final String severityString) {
         return this;
     }
 
+    /**
+     * Sets the detailed message for this issue.
+     *
+     * @param message
+     *         the message
+     *
+     * @return this
+     */
+    @CanIgnoreReturnValue
+    public IssueBuilder setMessage(final StringBuilder message) {
+        return setMessage(message.toString());
+    }
+
     /**
      * Sets the detailed message for this issue.
      *
@@ -588,6 +601,30 @@ public IssueBuilder addLocation(final Location location) {
         return this;
     }
 
+    /**
+     * Adds another location to this issue, the first location is considered the primary location.
+     *
+     * @param fileName
+     *         the name of the affected file
+     * @param lineStart
+     *         the first line of the affected code
+     * @param lineEnd
+     *         the last line of the affected code
+     * @param columnStart
+     *         the first column of the affected code
+     * @param columnEnd
+     *         the last column of the affected code
+     *
+     * @return this
+     */
+    @CanIgnoreReturnValue
+    @SuppressWarnings("checkstyle:HiddenField")
+    public IssueBuilder addLocation(final String fileName,
+            final int lineStart, final int lineEnd,
+            final int columnStart, final int columnEnd) {
+        return addLocation(new Location(internFileName(fileName), lineStart, lineEnd, columnStart, columnEnd));
+    }
+
     /**
      * Adds another location to this issue, the first location is considered the primary location.
      *
diff --git a/src/main/java/edu/hm/hafner/analysis/parser/Gcc4CompilerParser.java b/src/main/java/edu/hm/hafner/analysis/parser/Gcc4CompilerParser.java
@@ -4,14 +4,12 @@
 
 import edu.hm.hafner.analysis.Issue;
 import edu.hm.hafner.analysis.IssueBuilder;
-import edu.hm.hafner.analysis.Location;
 import edu.hm.hafner.analysis.LookaheadParser;
-import edu.hm.hafner.analysis.Severity;
 import edu.hm.hafner.analysis.util.IntegerParser;
 import edu.hm.hafner.util.LookaheadStream;
-import edu.hm.hafner.util.TreeStringBuilder;
 
 import java.io.Serial;
+import java.util.ArrayList;
 import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -25,12 +23,13 @@ public class Gcc4CompilerParser extends LookaheadParser {
     @Serial
     private static final long serialVersionUID = 5490211629355204910L;
 
+    private static final String LOCATION = "(?<file>.+?):(?<line>\\d+):(?:(?<column>\\d+):)?";
     private static final String GCC_WARNING_PATTERN =
-            ANT_TASK + "(.+?):(\\d+):(?:(\\d+):)? ?([wW]arning|.*[Ee]rror|[Nn]ote): (.*)$";
+            ANT_TASK + LOCATION + " ?(?<severity>[wW]arning|.*[Ee]rror): (?<message>.*)$";
     private static final Pattern CLASS_PATTERN = Pattern.compile("\\[-W(.+)]$");
     private static final Pattern GCC_OPTION_PATTERN = Pattern.compile("\\[-(f[^\\]]+)]$");
     private static final Pattern CLANG_TIDY_PATTERN = Pattern.compile("\\[[^\\s\\]]+\\]$");
-    private static final Pattern NOTE_PATTERN = Pattern.compile("^(?<file>.+?):(?<line>\\d+):(?:(?<column>\\d+):)? ?[Nn]ote: (?<message>.*)$");
+    private static final Pattern NOTE_PATTERN = Pattern.compile("^" + LOCATION + " ?[Nn]ote: (?<message>.*)$");
 
     /**
      * Creates a new instance of {@link Gcc4CompilerParser}.
@@ -40,8 +39,10 @@ public Gcc4CompilerParser() {
     }
 
     /**
-     * Creates a new instance of {@link Gcc4CompilerParser} with specified pattern.
-     * @param pattern a regex pattern to be used instead of the default one
+     * Creates a new instance of {@link Gcc4CompilerParser} with the specified pattern.
+     *
+     * @param pattern
+     *         a regex pattern to be used instead of the default one
      */
     Gcc4CompilerParser(final String pattern) {
         super(pattern);
@@ -55,64 +56,69 @@ protected boolean isLineInteresting(final String line) {
     @Override
     protected Optional<Issue> createIssue(final Matcher matcher, final LookaheadStream lookahead,
             final IssueBuilder builder) {
-        if (isNoteMessage(matcher)) {
+        var originalMessage = matcher.group("message");
+        if (isClangTidyWarning(originalMessage)) {
             return Optional.empty();
         }
 
-        var message = new StringBuilder(matcher.group(5));
-        var originalMessage = message.toString();
-
-        setCategory(builder, originalMessage);
+        addLocation(matcher, builder);
 
+        var message = new StringBuilder(originalMessage);
         boolean hasCodeSnippet = false;
         while (lookahead.hasNext() && isMessageContinuation(lookahead, hasCodeSnippet)) {
             var continuation = lookahead.next();
-            if (continuation.length() > 0 && Character.isWhitespace(continuation.charAt(0))) {
+            if (!continuation.isEmpty() && Character.isWhitespace(continuation.charAt(0))) {
                 hasCodeSnippet = true;
             }
             message.append('\n');
             message.append(continuation);
         }
 
-        var treeStringBuilder = new TreeStringBuilder();
-        var notes = collectNotes(lookahead, builder, treeStringBuilder);
+        var notes = collectNotes(lookahead, builder);
         if (!notes.isEmpty()) {
             message.append('\n');
             message.append(notes);
         }
 
-        // Reject clang-tidy warnings that have [check-name] but not [-Wwarning-name]
-        // clang-tidy warnings should be handled by ClangTidyParser
-        var messageStr = message.toString();
-        if (CLANG_TIDY_PATTERN.matcher(messageStr).find() && !CLASS_PATTERN.matcher(messageStr).find()) {
-            return Optional.empty();
-        }
-
-        return builder.setFileName(matcher.group(1))
-                .setLineStart(matcher.group(2))
-                .setColumnStart(matcher.group(3))
-                .setMessage(messageStr)
-                .setSeverity(Severity.guessFromString(matcher.group(4)))
+        setCategory(builder, originalMessage);
+        return builder.setMessage(message)
+                .guessSeverity(matcher.group("severity"))
                 .buildOptional();
     }
 
+    private void addLocation(final Matcher matcher, final IssueBuilder builder) {
+        var fileName = matcher.group("file");
+        var line = toInt(matcher.group("line"));
+        var column = toInt(matcher.group("column"));
+
+        builder.addLocation(fileName, line, line, column, column);
+    }
+
+    private int toInt(final String number) {
+        return IntegerParser.parseInt(number);
+    }
+
     /**
-     * Checks if the matched line is a note message.
+     * Check if the warning is from clang-tidy. Those warnings are quite similar and have [check-name] but not
+     * [-Wwarning-name] or [-fcompiler-option]. Since a separate parser handles clang-tidy warnings, we can skip them
+     * here.
      *
-     * @param matcher the matcher for the current line
-     * @return true if the line is a note message, false otherwise
+     * @return {@code true} if the message is a clang-tidy warning, {@code false} otherwise
      */
-    private boolean isNoteMessage(final Matcher matcher) {
-        var messageType = matcher.group(4);
-        return "note".equals(messageType) || "Note".equals(messageType);
+    private boolean isClangTidyWarning(final String message) {
+        return CLANG_TIDY_PATTERN.matcher(message).find()
+                && !CLASS_PATTERN.matcher(message).find()
+                && !GCC_OPTION_PATTERN.matcher(message).find();
     }
 
     /**
-     * Sets the category based on the original message content.
-     * Checks for GCC warning categories (e.g., [-Wunused-variable]) or compiler options (e.g., [-fpermissive]).
+     * Sets the category based on the original message content. Checks for GCC warning categories (e.g.,
+     * [-Wunused-variable]) or compiler options (e.g., [-fpermissive]).
      *
-     * @param builder the issue builder
-     * @param originalMessage the original message to extract the category from
+     * @param builder
+     *         the issue builder
+     * @param originalMessage
+     *         the original message to extract the category from
      */
     private void setCategory(final IssueBuilder builder, final String originalMessage) {
         var classMatcher = CLASS_PATTERN.matcher(originalMessage);
@@ -128,38 +134,27 @@ private void setCategory(final IssueBuilder builder, final String originalMessag
     }
 
     /**
-     * Collects any subsequent note lines from the lookahead stream and adds them as additional locations.
+     * Collects any further note lines from the lookahead stream and adds them as additional locations.
      *
-     * @param lookahead the lookahead stream
-     * @param builder the issue builder to add locations to
-     * @param treeStringBuilder the tree string builder for interning file names
+     * @param lookahead
+     *         the lookahead stream
+     * @param builder
+     *         the issue builder to add locations to
      * @return a string containing all collected note lines, or an empty string if no notes were found
      */
-    private String collectNotes(final LookaheadStream lookahead, final IssueBuilder builder,
-            final TreeStringBuilder treeStringBuilder) {
-        var notes = new StringBuilder();
+    private String collectNotes(final LookaheadStream lookahead, final IssueBuilder builder) {
+        var notes = new ArrayList<String>();
         while (lookahead.hasNext()) {
-            var nextLine = lookahead.peekNext();
-            var noteMatcher = NOTE_PATTERN.matcher(nextLine);
+            var noteMatcher = NOTE_PATTERN.matcher(lookahead.peekNext());
             if (noteMatcher.matches()) {
-                if (!notes.isEmpty()) {
-                    notes.append('\n');
-                }
-                notes.append(lookahead.next());
-
-                // Add the note as an additional location
-                var file = noteMatcher.group("file");
-                var line = IntegerParser.parseInt(noteMatcher.group("line"));
-                var column = IntegerParser.parseInt(noteMatcher.group("column"));
-
-                var fileName = treeStringBuilder.intern(file);
-                builder.addLocation(new Location(fileName, line, line, column, column));
+                notes.add(lookahead.next());
+                addLocation(noteMatcher, builder);
             }
             else {
                 break;
             }
         }
-        return notes.toString();
+        return String.join("\n", notes);
     }
 
     @SuppressWarnings("PMD.AvoidLiteralsInIfCondition")
diff --git a/src/test/java/edu/hm/hafner/analysis/parser/Gcc4CompilerParserTest.java b/src/test/java/edu/hm/hafner/analysis/parser/Gcc4CompilerParserTest.java
@@ -622,11 +622,40 @@ void issue55221() {
         }
     }
 
-    /**
-     * Parses a javac warning log.
-     *
-     * @see <a href="https://issues.jenkins.io/browse/JENKINS-68396">Issue 68396</a>
-     */
+    @Test
+    @org.junitpioneer.jupiter.Issue("https://github.com/jenkinsci/analysis-model/issues/1488")
+    void shouldRegisterPrimaryLocationInTheBeginning() {
+        var report = parseStringContent(""" 
+                /workspace/foo.cpp:134:26: warning: declaration of \\'i\\' shadows a previous local [-Wshadow]
+                  134 |                 for (u32 i = 0; i < (cmd.Size-sizeof(TDSPCommand))/4 && (cmd.Size>sizeof(TDSPCommand))>0; i++)
+                      |                          ^
+                /workspace/foo.cpp:122:18: note: shadowed declaration is here
+                  122 |         for (int i = 0; i < _commandsCount; i++)
+                      |                  ^
+                """);
+
+        assertThat(report).hasSize(1);
+
+        var issue = report.get(0);
+        assertThat(issue)
+                .hasLineStart(134)
+                .hasColumnStart(26)
+                .hasFileName("/workspace/foo.cpp")
+                .hasSeverity(Severity.WARNING_NORMAL);
+        assertThat(issue.getMessage()).containsIgnoringWhitespaces("""
+                declaration of \\'i\\' shadows a previous local [-Wshadow]
+                       134 |                 for (u32 i = 0; i < (cmd.Size-sizeof(TDSPCommand))/4 && (cmd.Size>sizeof(TDSPCommand))>0; i++)
+                           |                          ^
+                     /workspace/foo.cpp:122:18: note: shadowed declaration is here
+                """);
+        assertThat(issue.getLocations()).hasSize(2);
+        assertThat(issue.getLocations().getLast())
+                .hasFileName("/workspace/foo.cpp")
+                .hasLineStart(122)
+                .hasColumnStart(18);
+    }
+
+    @org.junitpioneer.jupiter.Issue("https://issues.jenkins.io/browse/JENKINS-68396")
     @Test
     void issue68396() {
         var warnings = parse("issue68396.txt");
