Skip to content

Commit cd33fbf

Browse files
ahoppenahoppen
committed
Generalize the removal of compiler argument options during indexing
The existing ad-hoc logic was not quite correct because it didn’t eg. remove `-MT/depfile` because it assumed that `-MT` was followed by a space. It also didn’t take into account that `serialize-diagnostics` can be spelled with a single dash or two dashes. Create a `CompilerCommandLineOption` type that forces decisions to be made about the dash spelling and argument styles, which should help avoid problems like this in the future.
1 parent 490871b commit cd33fbf

File tree

5 files changed

+253
-53
lines changed

5 files changed

+253
-53
lines changed

‎Package.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,13 @@ let package = Package(
182182
exclude: ["CMakeLists.txt"]
183183
),
184184

185+
.testTarget(
186+
name: "SemanticIndexTests",
187+
dependencies: [
188+
"SemanticIndex"
189+
]
190+
),
191+
185192
// MARK: SKCore
186193
// Data structures and algorithms useful across the project, but not necessarily
187194
// suitable for use in other packages.

‎Sources/SemanticIndex/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11

22
add_library(SemanticIndex STATIC
33
CheckedIndex.swift
4+
CompilerCommandLineOption.swift
45
IndexTaskDescription.swift
56
PreparationTaskDescription.swift
67
SemanticIndexManager.swift

‎Sources/SemanticIndex/CompilerCommandLineOption.swift

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
@_spi(Testing) public struct CompilerCommandLineOption {
14+
/// Return value of `matches(argument:)`.
15+
public enum Match {
16+
/// The `CompilerCommandLineOption` matched the command line argument. The next element in the command line is a
17+
/// separate argument and should not be removed.
18+
case removeOption
19+
20+
/// The `CompilerCommandLineOption` matched the command line argument. The next element in the command line is an
21+
/// argument to this option and should not be removed as well.
22+
case removeOptionAndNextArgument
23+
}
24+
25+
public enum DashSpelling {
26+
case singleDash
27+
case doubleDash
28+
}
29+
30+
public enum ArgumentStyles {
31+
/// A command line option where arguments can be passed with or without a space as `-MT/file.txt` of `-MT /file.txt`.
32+
case noSpace
33+
/// A command line option where the argument is passed, separated by a space (eg. `--serialize-diagnostics /file.txt`)
34+
case separatedBySpace
35+
/// A command line option where the argument is passed after a `=`, eg. `-fbuild-session-file=`.
36+
case separatedByEqualSign
37+
}
38+
39+
/// The name of the option, without any preceeding `-` or `--`.
40+
private let name: String
41+
42+
/// Whether the option can be spelled with one or two dashes.
43+
private let dashSpellings: [DashSpelling]
44+
45+
/// The ways that arguments can specified after the option. Empty if the option is a flag that doesn't take any
46+
/// argument.
47+
private let argumentStyles: [ArgumentStyles]
48+
49+
public static func flag(_ name: String, _ dashSpellings: [DashSpelling]) -> CompilerCommandLineOption {
50+
precondition(!dashSpellings.isEmpty)
51+
return CompilerCommandLineOption(name: name, dashSpellings: dashSpellings, argumentStyles: [])
52+
}
53+
54+
public static func option(
55+
_ name: String,
56+
_ dashSpellings: [DashSpelling],
57+
_ argumentStyles: [ArgumentStyles]
58+
) -> CompilerCommandLineOption {
59+
precondition(!dashSpellings.isEmpty)
60+
precondition(!argumentStyles.isEmpty)
61+
return CompilerCommandLineOption(name: name, dashSpellings: dashSpellings, argumentStyles: argumentStyles)
62+
}
63+
64+
public func matches(argument: String) -> Match? {
65+
let argumentName: Substring
66+
if argument.hasPrefix("--") {
67+
if dashSpellings.contains(.doubleDash) {
68+
argumentName = argument.dropFirst(2)
69+
} else {
70+
return nil
71+
}
72+
} else if argument.hasPrefix("-") {
73+
if dashSpellings.contains(.singleDash) {
74+
argumentName = argument.dropFirst(1)
75+
} else {
76+
return nil
77+
}
78+
} else {
79+
return nil
80+
}
81+
guard argumentName.hasPrefix(self.name) else {
82+
// Fast path in case the argument doesn't match.
83+
return nil
84+
}
85+
86+
// Examples:
87+
// - self.name: "emit-module", argument: "-emit-module", then textAfterArgumentName: ""
88+
// - self.name: "o", argument: "-o", then textAfterArgumentName: ""
89+
// - self.name: "o", argument: "-output-file-map", then textAfterArgumentName: "utput-file-map"
90+
// - self.name: "MT", argument: "-MT/path/to/depfile", then textAfterArgumentName: "/path/to/depfile"
91+
// - self.name: "fbuild-session-file", argument: "-fbuild-session-file=/path/to/file", then textAfterArgumentName: "=/path/to/file"
92+
let textAfterArgumentName: Substring = argumentName.dropFirst(self.name.count)
93+
94+
if argumentStyles.isEmpty {
95+
if textAfterArgumentName.isEmpty {
96+
return .removeOption
97+
} else {
98+
// The command line option is a flag but there is text remaining after the argument name. Thus the flag didn't
99+
// match. Eg. self.name: "o" and argument: "-output-file-map"
100+
return nil
101+
}
102+
}
103+
104+
for argumentStyle in argumentStyles {
105+
switch argumentStyle {
106+
case .noSpace where !textAfterArgumentName.isEmpty:
107+
return .removeOption
108+
case .separatedBySpace where textAfterArgumentName.isEmpty:
109+
return .removeOptionAndNextArgument
110+
case .separatedByEqualSign where textAfterArgumentName.hasPrefix("="):
111+
return .removeOption
112+
default:
113+
break
114+
}
115+
}
116+
return nil
117+
}
118+
}
119+
120+
extension Array<CompilerCommandLineOption> {
121+
func firstMatch(for argument: String) -> CompilerCommandLineOption.Match? {
122+
for optionToRemove in self {
123+
if let match = optionToRemove.matches(argument: argument) {
124+
return match
125+
}
126+
}
127+
return nil
128+
}
129+
}

‎Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift

Lines changed: 60 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -281,37 +281,39 @@ private func adjustSwiftCompilerArgumentsForIndexStoreUpdate(
281281
_ compilerArguments: [String],
282282
fileToIndex: DocumentURI
283283
) -> [String] {
284-
let removeFlags: Set<String> = [
285-
"-c",
286-
"-disable-cmo",
287-
"-emit-dependencies",
288-
"-emit-module-interface",
289-
"-emit-module",
290-
"-emit-module",
291-
"-emit-objc-header",
292-
"-incremental",
293-
"-no-color-diagnostics",
294-
"-parseable-output",
295-
"-save-temps",
296-
"-serialize-diagnostics",
297-
"-use-frontend-parseable-output",
298-
"-validate-clang-modules-once",
299-
"-whole-module-optimization",
300-
]
284+
let optionsToRemove: [CompilerCommandLineOption] = [
285+
.flag("c", [.singleDash]),
286+
.flag("disable-cmo", [.singleDash]),
287+
.flag("emit-dependencies", [.singleDash]),
288+
.flag("emit-module-interface", [.singleDash]),
289+
.flag("emit-module", [.singleDash]),
290+
.flag("emit-module", [.singleDash]),
291+
.flag("emit-objc-header", [.singleDash]),
292+
.flag("incremental", [.singleDash]),
293+
.flag("no-color-diagnostics", [.singleDash]),
294+
.flag("parseable-output", [.singleDash]),
295+
.flag("save-temps", [.singleDash]),
296+
.flag("serialize-diagnostics", [.singleDash]),
297+
.flag("use-frontend-parseable-output", [.singleDash]),
298+
.flag("validate-clang-modules-once", [.singleDash]),
299+
.flag("whole-module-optimization", [.singleDash]),
301300

302-
let removeArguments: Set<String> = [
303-
"-clang-build-session-file",
304-
"-emit-module-interface-path",
305-
"-emit-module-path",
306-
"-emit-objc-header-path",
307-
"-emit-package-module-interface-path",
308-
"-emit-private-module-interface-path",
309-
"-num-threads",
310-
"-o",
311-
"-output-file-map",
301+
.option("clang-build-session-file", [.singleDash], [.separatedBySpace]),
302+
.option("emit-module-interface-path", [.singleDash], [.separatedBySpace]),
303+
.option("emit-module-path", [.singleDash], [.separatedBySpace]),
304+
.option("emit-objc-header-path", [.singleDash], [.separatedBySpace]),
305+
.option("emit-package-module-interface-path", [.singleDash], [.separatedBySpace]),
306+
.option("emit-private-module-interface-path", [.singleDash], [.separatedBySpace]),
307+
.option("num-threads", [.singleDash], [.separatedBySpace]),
308+
// Technically, `-o` and the output file don't need to be separated by a space. Eg. `swiftc -oa file.swift` is
309+
// valid and will write to an output file named `a`.
310+
// We can't support that because the only way to know that `-output-file-map` is a different flag and not an option
311+
// to write to an output file named `utput-file-map` is to know all compiler arguments of `swiftc`, which we don't.
312+
.option("o", [.singleDash], [.separatedBySpace]),
313+
.option("output-file-map", [.singleDash], [.separatedBySpace, .separatedByEqualSign]),
312314
]
313315

314-
let removeFrontendFlags: Set<String> = [
316+
let removeFrontendFlags = [
315317
"-experimental-skip-non-inlinable-function-bodies",
316318
"-experimental-skip-all-function-bodies",
317319
]
@@ -320,12 +322,14 @@ private func adjustSwiftCompilerArgumentsForIndexStoreUpdate(
320322
result.reserveCapacity(compilerArguments.count)
321323
var iterator = compilerArguments.makeIterator()
322324
while let argument = iterator.next() {
323-
if removeFlags.contains(argument) {
325+
switch optionsToRemove.firstMatch(for: argument) {
326+
case .removeOption:
324327
continue
325-
}
326-
if removeArguments.contains(argument) {
328+
case .removeOptionAndNextArgument:
327329
_ = iterator.next()
328330
continue
331+
case nil:
332+
break
329333
}
330334
if argument == "-Xfrontend" {
331335
if let nextArgument = iterator.next() {
@@ -356,43 +360,46 @@ private func adjustClangCompilerArgumentsForIndexStoreUpdate(
356360
_ compilerArguments: [String],
357361
fileToIndex: DocumentURI
358362
) -> [String] {
359-
let removeFlags: Set<String> = [
363+
let optionsToRemove: [CompilerCommandLineOption] = [
360364
// Disable writing of a depfile
361-
"-M",
362-
"-MD",
363-
"-MMD",
364-
"-MG",
365-
"-MM",
366-
"-MV",
365+
.flag("M", [.singleDash]),
366+
.flag("MD", [.singleDash]),
367+
.flag("MMD", [.singleDash]),
368+
.flag("MG", [.singleDash]),
369+
.flag("MM", [.singleDash]),
370+
.flag("MV", [.singleDash]),
367371
// Don't create phony targets
368-
"-MP",
369-
// Don't writ out compilation databases
370-
"-MJ",
371-
// Continue in the presence of errors during indexing
372-
"-fmodules-validate-once-per-build-session",
372+
.flag("MP", [.singleDash]),
373+
// Don't write out compilation databases
374+
.flag("MJ", [.singleDash]),
373375
// Don't compile
374-
"-c",
375-
]
376+
.flag("c", [.singleDash]),
377+
378+
.flag("fmodules-validate-once-per-build-session", [.singleDash]),
376379

377-
let removeArguments: Set<String> = [
378380
// Disable writing of a depfile
379-
"-MT",
380-
"-MF",
381-
"-MQ",
381+
.option("MT", [.singleDash], [.noSpace, .separatedBySpace]),
382+
.option("MF", [.singleDash], [.noSpace, .separatedBySpace]),
383+
.option("MQ", [.singleDash], [.noSpace, .separatedBySpace]),
384+
382385
// Don't write serialized diagnostic files
383-
"--serialize-diagnostics",
386+
.option("serialize-diagnostics", [.singleDash, .doubleDash], [.separatedBySpace]),
387+
388+
.option("fbuild-session-file", [.singleDash], [.separatedByEqualSign]),
384389
]
385390

386391
var result: [String] = []
387392
result.reserveCapacity(compilerArguments.count)
388393
var iterator = compilerArguments.makeIterator()
389394
while let argument = iterator.next() {
390-
if removeFlags.contains(argument) || argument.starts(with: "-fbuild-session-file=") {
395+
switch optionsToRemove.firstMatch(for: argument) {
396+
case .removeOption:
391397
continue
392-
}
393-
if removeArguments.contains(argument) {
398+
case .removeOptionAndNextArgument:
394399
_ = iterator.next()
395400
continue
401+
case nil:
402+
break
396403
}
397404
result.append(argument)
398405
}

‎Tests/SemanticIndexTests/CompilerCommandLineOptionMatchingTests.swift

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2019 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
@_spi(Testing) import SemanticIndex
14+
import XCTest
15+
16+
final class CompilerCommandLineOptionMatchingTests: XCTestCase {
17+
func testFlags() {
18+
assertOption(.flag("a", [.singleDash]), "-a", .removeOption)
19+
assertOption(.flag("a", [.doubleDash]), "--a", .removeOption)
20+
assertOption(.flag("a", [.singleDash, .doubleDash]), "-a", .removeOption)
21+
assertOption(.flag("a", [.singleDash, .doubleDash]), "--a", .removeOption)
22+
assertOption(.flag("a", [.singleDash]), "-another", nil)
23+
assertOption(.flag("a", [.singleDash]), "--a", nil)
24+
assertOption(.flag("a", [.doubleDash]), "-a", nil)
25+
}
26+
27+
func testOptions() {
28+
assertOption(.option("a", [.singleDash], [.noSpace]), "-a/file.txt", .removeOption)
29+
assertOption(.option("a", [.singleDash], [.noSpace]), "-another", .removeOption)
30+
assertOption(.option("a", [.singleDash], [.separatedByEqualSign]), "-a=/file.txt", .removeOption)
31+
assertOption(.option("a", [.singleDash], [.separatedByEqualSign]), "-a/file.txt", nil)
32+
assertOption(.option("a", [.singleDash], [.separatedBySpace]), "-a", .removeOptionAndNextArgument)
33+
assertOption(.option("a", [.singleDash], [.separatedBySpace]), "-another", nil)
34+
assertOption(.option("a", [.singleDash], [.separatedBySpace]), "-a=/file.txt", nil)
35+
assertOption(.option("a", [.singleDash], [.noSpace, .separatedBySpace]), "-a/file.txt", .removeOption)
36+
assertOption(.option("a", [.singleDash], [.noSpace, .separatedBySpace]), "-a=/file.txt", .removeOption)
37+
assertOption(.option("a", [.singleDash], [.noSpace, .separatedBySpace]), "-a", .removeOptionAndNextArgument)
38+
assertOption(.option("a", [.singleDash], [.separatedByEqualSign, .separatedBySpace]), "-a/file.txt", nil)
39+
assertOption(.option("a", [.singleDash], [.separatedByEqualSign, .separatedBySpace]), "-a=file.txt", .removeOption)
40+
assertOption(
41+
.option("a", [.singleDash], [.separatedByEqualSign, .separatedBySpace]),
42+
"-a",
43+
.removeOptionAndNextArgument
44+
)
45+
}
46+
}
47+
48+
fileprivate func assertOption(
49+
_ option: CompilerCommandLineOption,
50+
_ argument: String,
51+
_ expected: CompilerCommandLineOption.Match?,
52+
file: StaticString = #filePath,
53+
line: UInt = #line
54+
) {
55+
XCTAssertEqual(option.matches(argument: argument), expected, file: file, line: line)
56+
}

0 commit comments

Comments
 (0)