remove the directive from the comments in the AST

Signed-off-by: Talon Bowler <[email protected]>

Author: daghack

frontend/dockerfile/dockerfile_lint_test.go

@@ -338,35 +338,30 @@ COPY $bar .
 
 func testRuleCheckOption(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`#check=skip=all
-#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=all;error=true
-#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing
-#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing;error=true
-#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing
-#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -378,14 +373,13 @@ copy Dockerfile .
 				Description: "The 'as' keyword should match the case of the 'from' keyword",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
 				Detail:      "'as' and 'FROM' keywords' casing do not match",
-				Line:        3,
+				Line:        2,
 				Level:       1,
 			},
 		},
 	})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing;error=true
-#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -397,7 +391,7 @@ copy Dockerfile .
 				Description: "The 'as' keyword should match the case of the 'from' keyword",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
 				Detail:      "'as' and 'FROM' keywords' casing do not match",
-				Line:        3,
+				Line:        2,
 				Level:       1,
 			},
 		},
@@ -407,7 +401,6 @@ copy Dockerfile .
 	})
 
 	dockerfile = []byte(`#check=skip=all
-#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -419,7 +412,7 @@ copy Dockerfile .
 				Description: "The 'as' keyword should match the case of the 'from' keyword",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
 				Detail:      "'as' and 'FROM' keywords' casing do not match",
-				Line:        3,
+				Line:        2,
 				Level:       1,
 			},
 		},
@@ -432,7 +425,6 @@ copy Dockerfile .
 	})
 
 	dockerfile = []byte(`#check=error=true
-#
 FROM scratch as base
 copy Dockerfile .
 `)

frontend/dockerfile/parser/parser.go

@@ -167,16 +167,17 @@ func (d *directives) setEscapeToken(s string) error {
 
 // possibleParserDirective looks for parser directives, eg '# escapeToken=<char>'.
 // Parser directives must precede any builder instruction or other comments,
-// and cannot be repeated.
-func (d *directives) possibleParserDirective(line []byte) error {
+// and cannot be repeated. Returns true if a parser directive was found.
+func (d *directives) possibleParserDirective(line []byte) (bool, error) {
 	directive, err := d.parser.ParseLine(line)
 	if err != nil {
-		return err
+		return false, err
 	}
 	if directive != nil && directive.Name == keyEscape {
-		return d.setEscapeToken(directive.Value)
+		err := d.setEscapeToken(directive.Value)
+		return err == nil, err
 	}
-	return nil
+	return directive != nil, nil
 }
 
 // newDefaultDirectives returns a new directives structure with the default escapeToken token
@@ -300,7 +301,13 @@ func Parse(rwc io.Reader) (*Result, error) {
 				comments = append(comments, comment)
 			}
 		}
-		bytesRead, err = processLine(d, bytesRead, true)
+		var directiveOk bool
+		bytesRead, directiveOk, err = processLine(d, bytesRead, true)
+		// If the line is a directive, strip it from the comments
+		// so it doesn't get added to the AST.
+		if directiveOk {
+			comments = comments[:len(comments)-1]
+		}
 		if err != nil {
 			return nil, withLocation(err, currentLine, 0)
 		}
@@ -316,7 +323,7 @@ func Parse(rwc io.Reader) (*Result, error) {
 
 		var hasEmptyContinuationLine bool
 		for !isEndOfLine && scanner.Scan() {
-			bytesRead, err := processLine(d, scanner.Bytes(), false)
+			bytesRead, _, err := processLine(d, scanner.Bytes(), false)
 			if err != nil {
 				return nil, withLocation(err, currentLine, 0)
 			}
@@ -527,12 +534,13 @@ func trimContinuationCharacter(line []byte, d *directives) ([]byte, bool) {
 
 // TODO: remove stripLeftWhitespace after deprecation period. It seems silly
 // to preserve whitespace on continuation lines. Why is that done?
-func processLine(d *directives, token []byte, stripLeftWhitespace bool) ([]byte, error) {
+func processLine(d *directives, token []byte, stripLeftWhitespace bool) ([]byte, bool, error) {
 	token = trimNewline(token)
 	if stripLeftWhitespace {
 		token = trimLeadingWhitespace(token)
 	}
-	return trimComments(token), d.possibleParserDirective(token)
+	directiveOk, err := d.possibleParserDirective(token)
+	return trimComments(token), directiveOk, err
 }
 
 // Variation of bufio.ScanLines that preserves the line endings