Do stricter checking of -D command-line arguments #6813
Conversation
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
just a drive by smoke test. Jordan is the right person to review this. |
|
@gottesmm : should I always mention @jrose-apple when submitting merge requests? |
|
Not always. Depends on the area. This is a change to the driver. Jordan wrote most of it IIRC, so he is the right person to review. |
|
Also, if you are unsure of who would be good to look at something, feel free to ask. I am sure someone will chime in = ). |
lib/Frontend/CompilerInvocation.cpp
Outdated
| if (Lexer::isIdentifier(name)) | ||
| Opts.addCustomConditionalCompilationFlag(name); | ||
| else if (name.find('=') != StringRef::npos) | ||
| Diags.diagnose(SourceLoc(), |
There was a problem hiding this comment.
This is something we should diagnose up at the Driver level, so that it happens once up front instead of once per input file in the module. I suggest adding it to validateArgs in Driver.cpp
There was a problem hiding this comment.
Hmm, but then it won't be used with -frontend specified.
There was a problem hiding this comment.
That's not a supported configuration anyway. Anyone using -frontend is supposed to be a compiler developer already. I suppose we could assert it, though, just to have a weak catch.
| @@ -168,6 +168,12 @@ ERROR(error_formatting_multiple_file_ranges,none, | |||
| ERROR(error_formatting_invalid_range,none, | |||
| "file range is invalid", ()) | |||
|
|
|||
| ERROR(invalid_conditional_compilation_flag,none, | |||
| "invalid conditional compilation flag in '-D%0'", (StringRef)) | |||
There was a problem hiding this comment.
Hm. In C the convention is to write -DFOO, but I'm not sure Swift should be encouraging that. -D FOO is equivalent and a bit easier to read.
The error message should also explain what the problem is, perhaps something like "conditional compilation flags must be valid Swift identifiers (rather than '%0')"?
| "invalid conditional compilation flag in '-D%0'", (StringRef)) | ||
|
|
||
| ERROR(cannot_assign_value_to_conditional_compilation_flag,none, | ||
| "conditional compilation flags cannot get a specific value assigned in '-D%0'", (StringRef)) |
There was a problem hiding this comment.
Nitpick: the grammar sounds a little funny to me. Maybe just "conditional compilation flags do not have values in Swift; they are either present or absent". Though I suppose the value the user provided has to be worked in there somewhere, since otherwise they won't know what it's referring to.
| @@ -6,3 +6,9 @@ | |||
| // RUN: not %swiftc_driver -c %s -o %t.o -Xfrontend -fake-frontend-arg -Xfrontend fakevalue 2>&1 | %FileCheck -check-prefix=XFRONTEND %s | |||
|
|
|||
| // XFRONTEND: <unknown>:0: error: unknown argument: '-fake-frontend-arg' | |||
|
|
|||
| // RUN: not %swift -D@#%! -DSwift=Cool -D-D -c %s -o %t.o 2>&1 | %FileCheck -check-prefix=CMDLNCOND %s | |||
There was a problem hiding this comment.
Can you add a test (or even just an argument) that shows the split -D, as mentioned above?
(Also, nitpick: there's no need to pick a compact name for the FileCheck prefix. "INVALID-COND" or something is fine.)
|
And for your other question: the CODE_OWNERS document kind of covers who's in charge of what, but some things fall in the cracks. "Argument validation" is considered a "Driver"-like task even though some of it happens in the Frontend library. |
|
I have a rebased version with most comments worked in, but I'm wondering if I should move the checks to |
Check if the condition "name" is a proper identifier, and generate an error when assigning specific values in -D conditions. Fixes SR-2404.
|
@swift-ci Please smoke test |
|
Heh, looks like there was another test accidentally using this, probably just as a way to test that arbitrary data could be passed through. Can you update that? |
|
Hey, Erik. Are you still working on this one? |
|
Rebased and merged in #10248 |
Check if the condition "name" is a proper identifier, and generate an
error when assigning specific values in -D conditions.
Fixes SR-2404.