Suggest to remove prefix `b` in cfg attribute lint string #54929

csmoe · 2018-10-09T09:57:07Z

Havvy · 2018-10-09T10:01:51Z

Woah, you're fast! Though I do wonder why there's no associated test with this change?

petrochenkov · 2018-10-09T10:12:37Z

These numerous ad-hoc "E-Easy" diagnostic PRs steadily fill the compiler with garbage that someone will have to eventually clean up.
This change in particular can be generalized, moved into a separate method, and applied to all cases where string literals are expected in attributes, bringing more benefit and less technical debt.

bors · 2018-10-11T09:26:56Z

☔ The latest upstream changes (presumably #54969) made this pull request unmergeable. Please resolve the merge conflicts.

estebank

The code change looks good to me, it doesn't seem to be introducing much more complexity than is already there, although I agree that this could potentially be generalized to all cases where a byte string is encountered where a &str is expected. I believe we already have ad-hoc handling for some of the cases where that happens, but having a centralized methodology would be useful.

With this said, I do fall on the camp of "a bit of tech-debt for huge usability gains are worth it", but the gains need to be extensive. This case in particular doesn't seem to be something that will be stumbled upon by somebody learning the language before learning the difference between "" and b"", so I could be easily convinced of not merging this PR and wait for the general solution (although part of the change actually cleans stuff up).

@petrochenkov do you want to take on the review for this PR? I'd defer to your final choice. As for generalizing, I feel that we can move a lot of these ad-hoc diagnostics further up in the parser by having a more complex version of Parser::eat(&mut self, TokenKind) that also takes some textual info for diagnostics, and also checks for "confusables"—not only single tokens that look alike, like ; and :, but also cases like this one (b""/""). What do you think?

estebank · 2018-10-12T17:13:56Z

src/test/ui/cfg-attr-syntax-validation.stderr

  --> $DIR/cfg-attr-syntax-validation.rs:22:11
   |
-LL | #[cfg(a = 10)] //~ ERROR literal in `cfg` predicate value must be a string
+LL | #[cfg(a = 10)] //~ ERROR unsupported literal


This change feels like a slight step backwards, from a very specific diagnostic towards a more general one... Shouldn't be a blocker.

Havvy · 2018-10-17T08:35:16Z

src/libsyntax/ast.rs

@@ -1326,6 +1326,14 @@ impl LitKind {
        }
    }

+    /// Returns true if this literal is byte literal string false otherwise.
+    pub fn is_bytestr(&self) -> bool {
+        match self {


Is there a reason not to use if let here?

petrochenkov · 2018-10-19T15:52:31Z

@estebank

As for generalizing, I feel that we can move a lot of these ad-hoc diagnostics further up in the parser by having a more complex version of Parser::eat(&mut self, TokenKind) that also takes some textual info for diagnostics, and also checks for "confusables"—not only single tokens that look alike, like ; and :, but also cases like this one (b""/""). What do you think?

I don't think this needs to go into parser?
You normally parse attribute contents as a meta-item, and only then inspect that meta-item to find necessary data and possibly report semantic errors like str literal vs bytestr literal.

petrochenkov · 2018-10-19T15:58:19Z

src/libsyntax/attr/builtin.rs

@@ -27,7 +27,7 @@ enum AttrError {
    UnsupportedLiteral
 }

-fn handle_errors(diag: &Handler, span: Span, error: AttrError) {
+fn handle_errors(diag: &Handler, span: Span, error: AttrError, is_bytestr: bool) {


is_bytestr is only used with AttrError::UnsupportedLiteral, so it can be made its part - AttrError::UnsupportedLiteral(bool /* is_bytestr */)

petrochenkov · 2018-10-19T16:03:06Z

src/libsyntax/attr/builtin.rs

@@ -461,8 +490,13 @@ pub fn cfg_matches(cfg: &ast::MetaItem, sess: &ParseSess, features: Option<&Feat
            MetaItemKind::List(..) => {
                error(cfg.span, "unexpected parentheses after `cfg` predicate key")
            }
-            MetaItemKind::NameValue(lit) if !lit.node.is_str() => {
-                error(lit.span, "literal in `cfg` predicate value must be a string")


Could you keep the old wording?
AttrError::UnsupportedLiteral can be extended to keep string fragments, and the resulting error message can be combined from them:

AttrError::UnsupportedLiteral(where, expected) span_err("literal in {} must be a {}", where, expected) AttrError::UnsupportedLiteral("`cfg` predicate value", "string")

It would give other UnsupportedLiteral errors better messages as well.

petrochenkov · 2018-10-19T16:10:05Z

I've left a couple of comments, otherwise LGTM.

rust-highfive · 2018-10-22T20:51:59Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:47:10] .................................................................................................... 300/4933
[00:47:12] .................................................................................................... 400/4933
[00:47:15] .................................................................................................... 500/4933
[00:47:19] ....................i............................................................................... 600/4933
[00:47:23] .............F...................................................................................... 700/4933
[00:47:31] ..........................................................................iiiii..................... 900/4933
[00:47:34] .................................................................................................... 1000/4933
[00:47:37] .................................................................................................... 1100/4933
[00:47:39] .................................................................................................... 1200/4933
---
[00:49:34] failures:
[00:49:34] 
[00:49:34] ---- [ui] ui/conditional-compilation/cfg-attr-syntax-validation.rs stdout ----
[00:49:34] 
[00:49:34] error: /checkout/src/test/ui/conditional-compilation/cfg-attr-syntax-validation.rs:25: unexpected error: '25:11: 25:16: literal in `cfg` predicate value must be a string [E0565]'
[00:49:34] error: 1 unexpected errors found, 0 expected errors not found
[00:49:34] status: exit code: 1
[00:49:34] status: exit code: 1
[00:49:34] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/conditional-compilation/cfg-attr-syntax-validation.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/conditional-compilation/cfg-attr-syntax-validation/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/conditional-compilation/cfg-attr-syntax-validation/auxiliary" "-A" "unused"
[00:49:34]     Error {
[00:49:34]         line_num: 25,
[00:49:34]         kind: Some(
[00:49:34]             Error
[00:49:34]             Error
[00:49:34]         ),
[00:49:34]         msg: "25:11: 25:16: literal in `cfg` predicate value must be a string [E0565]"
[00:49:34] ]
[00:49:34] 
[00:49:34] thread '[ui] ui/conditional-compilation/cfg-attr-syntax-validation.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1358:13
[00:49:34] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:49:34] 
[00:49:34] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:503:22
[00:49:34] 
[00:49:34] 
[00:49:34] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-5.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "5.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:49:34] 
[00:49:34] 
[00:49:34] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:49:34] Build completed unsuccessfully in 0:03:29
[00:49:34] Build completed unsuccessfully in 0:03:29
[00:49:34] Makefile:58: recipe for target 'check' failed
[00:49:34] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0f635190
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0c376004:start=1540236867030322347,finish=1540236867034409500,duration=4087153
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:11cb9080
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0792a006
travis_time:start:0792a006
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0ef67a4d
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/test/ui/conditional-compilation/cfg-attr-syntax-validation.rs

src/test/ui/conditional-compilation/cfg-attr-syntax-validation.stderr

Co-Authored-By: csmoe <csmoe@msn.com>

petrochenkov · 2018-10-23T10:27:11Z

@bors r+

bors · 2018-10-23T10:27:18Z

📌 Commit 81a609b has been approved by petrochenkov

bors · 2018-10-23T13:34:28Z

⌛ Testing commit 81a609b with merge 5d65ad1b67c016e67e7ffde084d2d33a830ed34a...

bors · 2018-10-23T15:52:08Z

💔 Test failed - status-travis

kennytm · 2018-10-23T16:57:36Z

@bors retry #43283

bors · 2018-10-26T21:46:21Z

⌛ Testing commit 81a609b with merge fa45602...

@estebank

Suggest to remove prefix `b` in cfg attribute lint string Closes #54926 r? @estebank

bors · 2018-10-27T00:39:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing fa45602 to master...

rust-highfive assigned estebank Oct 9, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2018

This comment has been minimized.

Sign in to view

Havvy mentioned this pull request Oct 9, 2018

Better error message using byte string where only regular strings are allowed in attributes #54926

Closed

csmoe force-pushed the cfg_lint branch 2 times, most recently from eac0b61 to 9059b49 Compare October 9, 2018 11:13

This comment has been minimized.

Sign in to view

csmoe force-pushed the cfg_lint branch 3 times, most recently from 39068c7 to e48b515 Compare October 9, 2018 12:15

This comment has been minimized.

Sign in to view

csmoe force-pushed the cfg_lint branch 2 times, most recently from 859f31c to e516b6f Compare October 9, 2018 15:46

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2018

estebank reviewed Oct 12, 2018

View reviewed changes

petrochenkov self-assigned this Oct 13, 2018

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 13, 2018

Havvy reviewed Oct 17, 2018

View reviewed changes

petrochenkov reviewed Oct 19, 2018

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2018

update meta item checking test

fb7c76b

csmoe force-pushed the cfg_lint branch 2 times, most recently from 1f796b2 to 9038c71 Compare October 22, 2018 09:12

optimize unsupported literal diag message

a76690f

csmoe force-pushed the cfg_lint branch from 9038c71 to a76690f Compare October 22, 2018 12:28

This comment has been minimized.

Sign in to view

estebank reviewed Oct 22, 2018

View reviewed changes

src/test/ui/conditional-compilation/cfg-attr-syntax-validation.rs Outdated Show resolved Hide resolved

estebank reviewed Oct 22, 2018

View reviewed changes

src/test/ui/conditional-compilation/cfg-attr-syntax-validation.stderr Outdated Show resolved Hide resolved

estebank and others added 2 commits October 23, 2018 09:03

add expected error comment

82b86e5

Co-Authored-By: csmoe <csmoe@msn.com>

add expected error comment

81a609b

Co-Authored-By: csmoe <csmoe@msn.com>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 23, 2018

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 23, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2018

bors added a commit that referenced this pull request Oct 26, 2018

Auto merge of #54929 - csmoe:cfg_lint, r=petrochenkov

fa45602

Suggest to remove prefix `b` in cfg attribute lint string Closes #54926 r? @estebank

bors merged commit 81a609b into rust-lang:master Oct 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest to remove prefix `b` in cfg attribute lint string #54929

Suggest to remove prefix `b` in cfg attribute lint string #54929

csmoe commented Oct 9, 2018

Havvy commented Oct 9, 2018

This comment has been minimized.

petrochenkov commented Oct 9, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Oct 11, 2018

estebank left a comment

estebank Oct 12, 2018

Havvy Oct 17, 2018

petrochenkov commented Oct 19, 2018 •

edited

Loading

petrochenkov Oct 19, 2018 •

edited

Loading

petrochenkov Oct 19, 2018 •

edited

Loading

petrochenkov Oct 19, 2018

petrochenkov commented Oct 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-highfive commented Oct 22, 2018

petrochenkov commented Oct 23, 2018

bors commented Oct 23, 2018

bors commented Oct 23, 2018

bors commented Oct 23, 2018

This comment has been minimized.

kennytm commented Oct 23, 2018

bors commented Oct 26, 2018

bors commented Oct 27, 2018

Suggest to remove prefix b in cfg attribute lint string #54929

Suggest to remove prefix b in cfg attribute lint string #54929

Conversation

csmoe commented Oct 9, 2018

Havvy commented Oct 9, 2018

This comment has been minimized.

petrochenkov commented Oct 9, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Oct 11, 2018

estebank left a comment

Choose a reason for hiding this comment

estebank Oct 12, 2018

Choose a reason for hiding this comment

Havvy Oct 17, 2018

Choose a reason for hiding this comment

petrochenkov commented Oct 19, 2018 • edited Loading

petrochenkov Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

petrochenkov Oct 19, 2018

Choose a reason for hiding this comment

petrochenkov commented Oct 19, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rust-highfive commented Oct 22, 2018

petrochenkov commented Oct 23, 2018

bors commented Oct 23, 2018

bors commented Oct 23, 2018

bors commented Oct 23, 2018

This comment has been minimized.

kennytm commented Oct 23, 2018

bors commented Oct 26, 2018

bors commented Oct 27, 2018

Suggest to remove prefix `b` in cfg attribute lint string #54929

Suggest to remove prefix `b` in cfg attribute lint string #54929

petrochenkov commented Oct 19, 2018 •

edited

Loading

petrochenkov Oct 19, 2018 •

edited

Loading

petrochenkov Oct 19, 2018 •

edited

Loading