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

Consider moving out-of-line module loading from parser to expansion #64197

Closed
petrochenkov opened this issue Sep 5, 2019 · 4 comments · Fixed by #69838
Closed

Consider moving out-of-line module loading from parser to expansion #64197

petrochenkov opened this issue Sep 5, 2019 · 4 comments · Fixed by #69838
Labels
A-frontend Area: frontend (errors, parsing and HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

Due to module loading parser has to do some not very appropriate for a parser things like

  • expanding cfgs on modules (duplicated with expansion)
  • keeping the current filesystem position (duplicated with expansion)

We also don't really need to load those files until we need to incorporate their contents into NodeId-entified AST, which happens during expansion.

The move will also mean never loading files in unconfigured code.
The loading is currently skipped only if cfg is placed directly on the mod m;, but not in other cases like

#[cfg(FALSE)]
mod m {
    mod n; // Still loaded
}
@petrochenkov petrochenkov added A-frontend Area: frontend (errors, parsing and HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2019
@matklad
Copy link
Member

matklad commented Sep 7, 2019

This is on my todo list, as it is a per-requisite for moving parser into a library.

@petrochenkov
Copy link
Contributor Author

The PR trying to split the libsyntax crate apart (#65324) has to use some hacks due to cfg-expansion being called from the parser.

@Mark-Simulacrum
Copy link
Member

#66565 removes those hacks in favor of a less principled approach (revealing the tight coupling between cfg expansion and parsing).

@Zoxc
Copy link
Contributor

Zoxc commented Jan 17, 2020

I would like the ability to start parsing immediately when we see a mod name;, but I guess the parser could just check if any #[cfg(..)] are in scope.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 13, 2020
expand: misc cleanups and simplifications

Some work I did while trying to understand expand for the purposes of rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 10, 2020
…ov,eddyb

Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 10, 2020
…ov,eddyb

Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 11, 2020
…ov,eddyb

Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 16, 2020
Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 17, 2020
Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Mar 17, 2020
Expansion-driven outline module parsing

After this PR, the parser will not do any conditional compilation or loading of external module files when `mod foo;` is encountered. Instead, the parser only leaves `mod foo;` in place in the AST, with no items filled in. Expansion later kicks in and will load the actual files and do the parsing. This entails that the following is now valid:

```rust
#[cfg(FALSE)]
mod foo {
    mod bar {
        mod baz; // `foo/bar/baz.rs` doesn't exist, but no error!
    }
}
```

Fixes rust-lang#64197.

r? @petrochenkov
@bors bors closed this as completed in 23b79d8 Mar 19, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 16, 2020
…e, r=petrochenkov

Cleanup stale 'FIXME(rust-lang#64197)'

(My first PR in rust-lang, any feedback is welcome. Please don't hesitate to let me know if I'm trying to do something pointless!)

Trivial cleanup of a stale `FIXME`, `StringReader.pos` is no longer exposed. For testing added `pos()` method that returns cloned value of `pos`.
bors added a commit to rust-lang-ci/rust that referenced this issue May 16, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#71662 (Implement FromStr for OsString)
 - rust-lang#71677 (Add explicit references to the BuildHasher trait)
 - rust-lang#71724 (Doc alias improvements)
 - rust-lang#71948 (Suggest to await future before ? operator)
 - rust-lang#72090 (rustc_driver: factor out computing the exit code)
 - rust-lang#72206 (Cleanup stale 'FIXME(rust-lang#64197)')
 - rust-lang#72218 (make sure even unleashed miri does not do pointer stuff)
 - rust-lang#72220 ([const-prop] Don't replace Rvalues that are already constants)
 - rust-lang#72224 (doc: add links to rotate_(left|right))

Failed merges:

r? @ghost
nnethercote added a commit to nnethercote/rust that referenced this issue May 3, 2024
There are some test cases involving `parse` and `tokenstream` and
`mut_visit` that are located in `rustc_expand`. Because it used to be
the case that constructing a `ParseSess` required the involvement of
`rustc_expand`. However, since rust-lang#64197 merged (a long time ago)
`rust_expand` no longer needs to be involved.

This commit moves the tests into `rustc_parse`. This is the optimal
place for the `parse` tests. It's not ideal for the `tokenstream` and
`mut_visit` tests -- they would be better in `rustc_ast` -- but they
still rely on parsing, which is not available in `rustc_ast`. But
`rustc_parse` is lower down in the crate graph and closer to `rustc_ast`
than `rust_expand`, so it's still an improvement for them.

The exact renaming is as follows:

- rustc_expand/src/mut_visit/tests.rs -> rustc_parse/src/parser/mut_visit/tests.rs
- rustc_expand/src/tokenstream/tests.rs -> rustc_parse/src/parser/tokenstream/tests.rs
- rustc_expand/src/tests.rs + rustc_expand/src/parse/tests.rs ->
  compiler/rustc_parse/src/parser/tests.rs

The latter two test files are combined because there's no need for them
to be separate, and having a `rustc_parse::parser::parse` module would
be weird. This also means some `pub(crate)`s can be removed.
nnethercote added a commit to nnethercote/rust that referenced this issue May 6, 2024
There are some test cases involving `parse` and `tokenstream` and
`mut_visit` that are located in `rustc_expand`. Because it used to be
the case that constructing a `ParseSess` required the involvement of
`rustc_expand`. However, since rust-lang#64197 merged (a long time ago)
`rust_expand` no longer needs to be involved.

This commit moves the tests into `rustc_parse`. This is the optimal
place for the `parse` tests. It's not ideal for the `tokenstream` and
`mut_visit` tests -- they would be better in `rustc_ast` -- but they
still rely on parsing, which is not available in `rustc_ast`. But
`rustc_parse` is lower down in the crate graph and closer to `rustc_ast`
than `rust_expand`, so it's still an improvement for them.

The exact renaming is as follows:

- rustc_expand/src/mut_visit/tests.rs -> rustc_parse/src/parser/mut_visit/tests.rs
- rustc_expand/src/tokenstream/tests.rs -> rustc_parse/src/parser/tokenstream/tests.rs
- rustc_expand/src/tests.rs + rustc_expand/src/parse/tests.rs ->
  compiler/rustc_parse/src/parser/tests.rs

The latter two test files are combined because there's no need for them
to be separate, and having a `rustc_parse::parser::parse` module would
be weird. This also means some `pub(crate)`s can be removed.
bors added a commit to rust-lang-ci/rust that referenced this issue May 6, 2024
…ler-errors

Move some tests from `rustc_expand` to `rustc_parse`.

There are some test cases involving `parse` and `tokenstream` and `mut_visit` that are located in `rustc_expand`. Because it used to be the case that constructing a `ParseSess` required the involvement of `rustc_expand`. However, since rust-lang#64197 merged (a long time ago) `rust_expand` no longer needs to be involved.

This commit moves the tests into `rustc_parse`. This is the optimal place for the `parse` tests. It's not ideal for the `tokenstream` and `mut_visit` tests -- they would be better in `rustc_ast` -- but they still rely on parsing, which is not available in `rustc_ast`. But `rustc_parse` is lower down in the crate graph and closer to `rustc_ast` than `rust_expand`, so it's still an improvement for them.

The exact renaming is as follows:

- rustc_expand/src/mut_visit/tests.rs -> rustc_parse/src/parser/mut_visit/tests.rs
- rustc_expand/src/tokenstream/tests.rs -> rustc_parse/src/parser/tokenstream/tests.rs
- rustc_expand/src/tests.rs + rustc_expand/src/parse/tests.rs -> compiler/rustc_parse/src/parser/tests.rs

The latter two test files are combined because there's no need for them to be separate, and having a `rustc_parse::parser::parse` module would be weird. This also means some `pub(crate)`s can be removed.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: frontend (errors, parsing and HIR) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants