Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve parse item fallback #125388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dev-ardi
Copy link
Contributor

@dev-ardi dev-ardi commented May 21, 2024

Closes #92615 and closes #101622
Please review the new diagnostics cafefully.

This PR also adds a few more comments to the parser.

@rustbot
Copy link
Collaborator

rustbot commented May 21, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 21, 2024
|
= note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>
= help: consider putting it inside a function: fn main() { [allow(unused_variables)] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think we want to have this recovery, at least when you could parse it like a macro attr w/o a #.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it thinks that it's an expression so it suggests putting it inside a function.
How can we tell that this is a bad attr and not an array literal with a function call?

Copy link
Member

@SparrowLii SparrowLii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR solves #101622 :(

.dcx()
.struct_span_err(self.token.span, format!("expected item, found {token_str}"));

match self.parse_full_stmt(AttemptLocalParseRecovery::No) {
Copy link
Member

@SparrowLii SparrowLii May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd be better doing more analysis to figure out what went wrong here, rather than treating them all as statements. For example, whether it contains ; in [] can be used to distinguish whether it is an attribute or an array.

));
}
StmtKind::MacCall(_) => {
err.span_label(span, "unexpected macro call at this position");
Copy link
Member

@SparrowLii SparrowLii May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least one example should be added for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean consider putting it inside a function?

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved

StmtKind::Semi(e) | StmtKind::Expr(e) => {
err.span_label(span, "unexpected expression").help(format!(
"consider putting it inside a function: fn main() {{ {} }}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tip also applies to the let statement above

@dev-ardi
Copy link
Contributor Author

I don't think this PR solves #101622 :(

It addresses the main point raised in the issue, right?

@dev-ardi dev-ardi force-pushed the improve-parse-item-fallback branch 3 times, most recently from 9a129e5 to 06a1ccf Compare May 31, 2024 15:29
@@ -76,6 +76,14 @@ pub fn attribute_to_string(attr: &ast::Attribute) -> String {
State::new().attribute_to_string(attr)
}

pub fn local_to_string(local: &ast::Local) -> String {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up being useless but I don't think that it's bad to have, right?

@dev-ardi
Copy link
Contributor Author

@rustbot ready
@SparrowLii I've addressed your concerns and reworked the approach a bit.

@bors
Copy link
Contributor

bors commented Jun 18, 2024

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

@dev-ardi dev-ardi force-pushed the improve-parse-item-fallback branch from 06a1ccf to 1d326d5 Compare June 19, 2024 20:19
@rust-log-analyzer

This comment has been minimized.

@dev-ardi
Copy link
Contributor Author

dev-ardi commented Jun 20, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2024
@dev-ardi dev-ardi force-pushed the improve-parse-item-fallback branch 2 times, most recently from 3f2ccd7 to b9ddf53 Compare June 20, 2024 22:31
@dev-ardi dev-ardi force-pushed the improve-parse-item-fallback branch from b9ddf53 to c66b446 Compare July 4, 2024 22:37
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 46)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 49)
 - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 61)
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

---- [ui] tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs stdout ----
diff of stdout:

- PRINT-BANG INPUT (DISPLAY): foo! { #[fake_attr] mod bar { #![doc = r" Foo"] } }
- PRINT-BANG DEEP-RE-COLLECTED (DISPLAY): foo! { #[fake_attr] mod bar { #! [doc = r" Foo"] } }
+ PRINT-BANG INPUT (DISPLAY): foo! { #[fake_attr] mod bar { #! [doc = r" Foo"] } }
3 PRINT-BANG INPUT (DEBUG): TokenStream [
5         ident: "foo",


The actual stdout differed from the expected stdout.
The actual stdout differed from the expected stdout.
Actual stdout saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/issue-78675-captured-inner-attrs/issue-78675-captured-inner-attrs.stdout
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args proc-macro/issue-78675-captured-inner-attrs.rs`

error: 1 errors occurred comparing output.
status: exit status: 0
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/issue-78675-captured-inner-attrs" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/proc-macro/issue-78675-captured-inner-attrs/auxiliary" "--edition=2018" "-Z" "span-debug"
--- stdout -------------------------------
PRINT-BANG INPUT (DISPLAY): foo! { #[fake_attr] mod bar { #! [doc = r" Foo"] } }
PRINT-BANG INPUT (DEBUG): TokenStream [
        ident: "foo",
        ident: "foo",
        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:20:9: 20:12 (#3),
    Punct {
    Punct {
        ch: '!',
        spacing: Alone,
        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:20:12: 20:13 (#3),
    Group {
        delimiter: Brace,
        stream: TokenStream [
            Punct {
            Punct {
                ch: '#',
                spacing: Alone,
                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:21:13: 21:14 (#3),
            Group {
                delimiter: Bracket,
                stream: TokenStream [
                    Ident {
                    Ident {
                        ident: "fake_attr",
                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:21:15: 21:24 (#3),
                ],
                ],
                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:21:14: 21:25 (#3),
            Group {
                delimiter: None,
                stream: TokenStream [
                    Ident {
                    Ident {
                        ident: "mod",
                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:27:5: 27:8 (#0),
                    Ident {
                        ident: "bar",
                        ident: "bar",
                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:27:9: 27:12 (#0),
                    Group {
                        delimiter: Brace,
                        stream: TokenStream [
                            Punct {
                            Punct {
                                ch: '#',
                                spacing: Joint,
                                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                            Punct {
                            Punct {
                                ch: '!',
                                spacing: Alone,
                                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                            Group {
                                delimiter: Bracket,
                                stream: TokenStream [
                                    Ident {
                                    Ident {
                                        ident: "doc",
                                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                                    Punct {
                                    Punct {
                                        ch: '=',
                                        spacing: Alone,
                                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                                    Literal {
                                        kind: StrRaw(0),
                                        symbol: " Foo",
                                        suffix: None,
                                        suffix: None,
                                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                                ],
                                ],
                                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:28:9: 28:16 (#0),
                        ],
                        ],
                        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:27:13: 29:6 (#0),
                ],
                ],
                span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:22:13: 22:18 (#3),
        ],
        ],
        span: /checkout/tests/ui/proc-macro/issue-78675-captured-inner-attrs.rs:20:14: 23:10 (#3),
]
------------------------------------------
stderr: none

@bors
Copy link
Contributor

bors commented Jul 18, 2024

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

@JohnCSimon
Copy link
Member

@dev-ardi
ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

@dev-ardi
Copy link
Contributor Author

dev-ardi commented Sep 3, 2024

It was left unreviewed for a while so it became full of conflicts. I intend to finish it at some point though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor recovery from pub let x = ... Provide a better error when code is put outside of a function
8 participants