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

Split libsyntax apart #65324

Merged
merged 3 commits into from
Nov 10, 2019
Merged

Split libsyntax apart #65324

merged 3 commits into from
Nov 10, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 12, 2019

In this PR the general idea is to separate the AST, parser, and friends by a more data / logic structure (tho not fully realized!) by separating out the parser and macro expansion code from libsyntax. Specifically have now three crates instead of one (libsyntax):

  • libsyntax:

    • concrete syntax tree (syntax::ast)

    • definition of tokens and token-streams (syntax::{token, tokenstream}) -- used by syntax::ast

    • visitors (syntax::visit, syntax::mut_visit)

    • shared definitions between libsyntax_expand

    • feature gating (syntax::feature_gate) -- we could possibly move this out to its own crater later.

    • attribute and meta item utilities, including used-marking (syntax::attr)

    • pretty printer (syntax::print) -- this should possibly be moved out later. For now I've reduced down the dependencies to a single essential one which could be broken via ParseSess. This entails that e.g. Debug impls for Path cannot reference the pretty printer.

    • definition of ParseSess (syntax::sess) -- this is used by syntax::{attr, print, feature_gate} and is a common definition used by the parser and other things like librustc.

    • the syntax::source_map -- this includes definitions used by syntax::ast and other things but could ostensibly be moved syntax_pos since that is more related to this module.

    • a smattering of misc utilities not sufficiently important to itemize -- some of these could be moved to where they are used (often a single place) but I wanted to limit the scope of this PR.

  • librustc_parse:

    • parser (rustc_parse::parser) -- reading a file and such are defined in the crate root tho.

    • lexer (rustc_parse::lexer)

    • validation of meta grammar (post-expansion) in (rustc_parse::validate_attr)

  • libsyntax_expand -- this defines the infra for macro expansion and conditional compilation but this is not libsyntax_ext; we might want to merge them later but currently libsyntax_expand is depended on by librustc_metadata which libsyntax_ext is not.

    • conditional compilation (syntax_expand::config) -- moved from syntax::config to here

    • the bulk of this crate is made up of the old syntax::ext

r? @estebank

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@petrochenkov petrochenkov self-assigned this Oct 12, 2019
@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2019
@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor

👍 on the general direction.
Some comments from a quick review:

  • The PR is already too big, please try to wrap it up without adding extra changes, unless they are necessary for splitting the crates.
  • The previous point also applies to local changes (like "simplify maybe_stage_features"), which are not necessary for splitting the crates and can be done later in a separate PR.
  • Please avoid tiny modules (like libsyntax_ext/util.rs, libsyntax/util/classify.rs), their contents can be moved into their parent modules.
  • syntax_macros -> syntax_expand, the former looks like it contains macros used in libsyntax.
  • syntax::ext -> syntax::expand, merge its tiny nested modules like base.rs and proc_macro.rs into it.
  • Style: please no multiline imports, a use item should start and end on the same line.
  • Moving attribute validation from PostExpansionVisitor::visit_attr to AST validation is incorrect, because this validation is done for inert attributes that have to disappear during macro expansion, like attributes on macro calls. See how PostExpansionVisitor::visit_attr is used from feature_gate::check::check_attribute which is called from expand.rs.
  • Please try to replace the FFI hacks connecting crates A and B with traits defined by their common dependencies. I'm not sure that the necessary crates are always linked together.

@petrochenkov 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 12, 2019
@petrochenkov
Copy link
Contributor

Regarding use of cfg-expansion from parse_item_mod - see #64197.

@Centril
Copy link
Contributor Author

Centril commented Oct 12, 2019

Some comments from a quick review:

Thanks for that :)

The PR is already too big, please try to wrap it up without adding extra changes, unless they are necessary for splitting the crates.

Yeah my goal with this PR was to propose a general direction and then split out smaller individual commits to merge first, especially for some of the larger or drive-by changes (like the one you referenced).

Please avoid tiny modules (like libsyntax_ext/util.rs, libsyntax/util/classify.rs), their contents can be moved into their parent modules.

Hmm... Some of these small modules were pre-existing so if possible I'd like to avoid more drive-by changes in this PR. :)

syntax_macros -> syntax_expand, the former looks like it contains macros used in libsyntax.

Nice! I'll use that. Better than my naming. :)

Style: please no multiline imports, a use item should start and end on the same line.

Will do.

Moving attribute validation from PostExpansionVisitor::visit_attr to AST validation is incorrect, because this validation is done for inert attributes that have to disappear during macro expansion, like attributes on macro calls. See how PostExpansionVisitor::visit_attr is used from feature_gate::check::check_attribute which is called from expand.rs.

Hmm... I inserted a call to validate_attr::check_meta(...); in expand.rs specifically to deal with this. Shouldn't that be sufficient? It feels very wrong to do validation logic in feature gating rather than a validation pass, so if AST validation is not good then I think we should add another pass just after feature gating. This is also a good idea because it doesn't create a cyclic dependency between the parser and feature gating.

Please try to replace the FFI hacks connecting crates A and B with traits defined by their common dependencies. I'm not sure that the necessary crates are always linked together.

By connecting them via e.g. ParseSess (in the parser) or Session (in lowering)? Sure, my initial idea was to do that using just function pointers (dyn Fn(...) -> ...) is unnecessary because these do not require state) but @Mark-Simulacrum suggested that this FFI hack is commonly used in C++-land for ThinLTO purposes but I guess there's not going to be notable perf differences either way from one indirect call here?

@Mark-Simulacrum
Copy link
Member

I only theorized about the FFI hack. I think traits are better, generally speaking, and using &dyn Trait -- maybe even &'static dyn Trait since we always go to a zero-sized struct implementing the trait or so makes a lot of sense.

@rust-highfive

This comment has been minimized.

Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
…h-ascription, r=davidtwco

syntax: simplify maybe_annotate_with_ascription

Split out from rust-lang#65324.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 13, 2019
syntax: consolidate function parsing in item.rs

Extracted from rust-lang#65324.

r? @estebank
@bors

This comment has been minimized.

also move MACRO_ARGUMENTS -> librustc_parse
@Centril Centril 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 Nov 10, 2019
@Centril
Copy link
Contributor Author

Centril commented Nov 10, 2019

Since this PR introduces syntax_parse, it probably makes sense to immediately introduce it as rustc_parse, crates introduced in the previous PRs can be renamed later (syntax_expand -> rustc_expand, syntax_ext -> rustc_builtin_macros, syntax/syntax_pos - ???).

Done. I would propose syntax -> rustc_ast and syntax_pos -> rustc_pos

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2019

📌 Commit 4ae2728 has been approved by petrochenkov

@bors 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 Nov 10, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Nov 10, 2019

@Centril

I would propose syntax -> rustc_ast and syntax_pos -> rustc_pos

rustc_pos -> rustc_span perhaps?

The crate consists of:

  • source map - kind of position, kind of span, spans and positions point into the source map
  • span - kind of position, literally span
  • hygiene - not a position, part of a span
  • edition - part of hygiene - not a position, part of a span
  • symbol/ident - neither position, nor span (ident has a span though)

@Centril
Copy link
Contributor Author

Centril commented Nov 10, 2019

rustc_pos -> rustc_span perhaps?

👍

@bors
Copy link
Contributor

bors commented Nov 10, 2019

⌛ Testing commit 4ae2728 with merge a3b6e57...

bors added a commit that referenced this pull request Nov 10, 2019
Split libsyntax apart

In this PR the general idea is to separate the AST, parser, and friends by a more data / logic structure (tho not fully realized!) by separating out the parser and macro expansion code from libsyntax. Specifically have now three crates instead of one (libsyntax):

- libsyntax:

   - concrete syntax tree (`syntax::ast`)

   - definition of tokens and token-streams (`syntax::{token, tokenstream}`) -- used by `syntax::ast`

   - visitors (`syntax::visit`, `syntax::mut_visit`)

   - shared definitions between `libsyntax_expand`

   - feature gating (`syntax::feature_gate`) -- we could possibly move this out to its own crater later.

   - attribute and meta item utilities, including used-marking (`syntax::attr`)

   - pretty printer (`syntax::print`) -- this should possibly be moved out later. For now I've reduced down the dependencies to a single essential one which could be broken via `ParseSess`. This entails that e.g. `Debug` impls for `Path` cannot reference the pretty printer.

   - definition of `ParseSess` (`syntax::sess`) -- this is used by `syntax::{attr, print, feature_gate}` and is a common definition used by the parser and other things like librustc.

   - the `syntax::source_map` -- this includes definitions used by `syntax::ast` and other things but could ostensibly be moved `syntax_pos` since that is more related to this module.

   - a smattering of misc utilities not sufficiently important to itemize -- some of these could be moved to where they are used (often a single place) but I wanted to limit the scope of this PR.

- librustc_parse:

   - parser (`rustc_parse::parser`) -- reading a file and such are defined in the crate root tho.

   - lexer (`rustc_parse::lexer`)

   - validation of meta grammar (post-expansion) in (`rustc_parse::validate_attr`)

- libsyntax_expand -- this defines the infra for macro expansion and conditional compilation but this is not libsyntax_ext; we might want to merge them later but currently libsyntax_expand is depended on by librustc_metadata which libsyntax_ext is not.

   - conditional compilation (`syntax_expand::config`) -- moved from `syntax::config` to here

   - the bulk of this crate is made up of the old `syntax::ext`

r? @estebank
@bors
Copy link
Contributor

bors commented Nov 10, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing a3b6e57 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 10, 2019
@bors bors merged commit 4ae2728 into rust-lang:master Nov 10, 2019
@Centril Centril deleted the organize-syntax branch November 10, 2019 15:55
bors added a commit to rust-lang/rust-clippy that referenced this pull request Nov 11, 2019
bors added a commit that referenced this pull request Dec 30, 2019
Rename some crates and modules in the frontend

Migrate from `syntax_*` naming scheme to `rustc_*`.
See #65324 (comment) and several comments below.

Renamed crates:
`syntax_expand` -> `rustc_expand`
`syntax_pos` -> `rustc_span` ([motivation](#65324 (comment)))
`syntax_ext` -> `rustc_builtin_macros`

Also one module in resolve is renamed for consistency and to avoid tautology.

r? @Centril
calebcartwright added a commit to calebcartwright/rustfmt that referenced this pull request Jan 16, 2020
several mods from libsyntax have been split out into separate crates,
including the `parse` mod
rust-lang/rust#65324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants