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

Modify Cargo.toml example to contain edition #711

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

alanwsmith
Copy link
Contributor

TL;DR

  • When I run cargo init it produces a Cargo.toml file with edition = "2021" in it.

  • When I went through Adding LALRPOP to your Cargo.toml file in the tutorial I only updated the [build-dependencies] and [dependencies] sections not thinking about the edition in the [package] section at all.

  • I got a bunch of errors shown below.

  • Three options that fixed the errors for me were:

    1: Changing the edition to "2018" in Cargo.toml

    2: Removing the edition from Cargo.toml

    3: Keeping the edition at "2021" and adding
    lalrpop = "0.19.8" to the [dependencies]
    in the Cargo.toml file

I'm still new to Rust, but I don't think I've customized cargo init in a way that makes it add the edition. I'm assuming it does that by default now. With that in mind I'm put together this PR which adds the edition to the example and puts lalrpop under the dependencies.


Steps to reproduce:

Step 1

Run:

cargo new --bin calculator

and cd into the directory with cd calculator

Step 2

Update the build-dependencies and dependencies
sections of the Cargo.toml file by modifying this
default file that was created:

[package]
name = "calculator"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

To:

[package]
name = "calculator"
version = "0.1.0"
edition = "2021"

[build-dependencies] # <-- We added this and everything after!
lalrpop = "0.19.8"

[dependencies]
lalrpop-util = "0.19.8"
regex = "1"

Step 3

Create build.rs with

extern crate lalrpop;

fn main() {
    lalrpop::process_root().unwrap();
}

Step 4

Create src/calculator1.lalrpop with:

use std::str::FromStr;

grammar;

pub Term: i32 = {
    <n:Num> => n,
    "(" <t:Term> ")" => t,
};

Num: i32 = <s:r"[0-9]+"> => i32::from_str(s).unwrap();

Step 5

Change the main.rs file to:

#[macro_use]
extern crate lalrpop_util;

fn main() {
    println!("Run `cargo test` to use this file");
}

lalrpop_mod!(pub calculator1); // synthesized by LALRPOP

#[test]
fn calculator1() {
    assert!(calculator1::TermParser::new()
        .parse("22")
        .is_ok());
    assert!(calculator1::TermParser::new()
        .parse("(22)")
        .is_ok());
    assert!(calculator1::TermParser::new()
        .parse("((((22))))")
        .is_ok());
    assert!(calculator1::TermParser::new()
        .parse("((22)")
        .is_err());
}

Step 6

Run:

cargo test

And see a bunch of errors like:

error[E0432]: unresolved import `self::__lalrpop_util::lexer`
   --> /Users/alan/Desktop/tmp/calculator/target/debug/build/calculator-4eb3fb97eeab7b69/out/calculator1.rs:388:38
    |
388 | pub(crate) use self::__lalrpop_util::lexer::Token;
    |                                      ^^^^^ could not find `lexer` in `__lalrpop_util`

error[E0433]: failed to resolve: could not find `lexer` in `__lalrpop_util`
   --> /Users/alan/Desktop/tmp/calculator/target/debug/build/calculator-4eb3fb97eeab7b69/out/calculator1.rs:217:34
    |
217 |         builder: __lalrpop_util::lexer::MatcherBuilder,
    |                                  ^^^^^ could not find `lexer` in `__lalrpop_util`

Fix

The option I chose to get the test to run is to change the Cargo.toml to add lalrpop to [dependencies] so the file changes from this:

[package]
name = "calculator"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[build-dependencies] # <-- We added this and everything after!
lalrpop = "0.19.8"

[dependencies]
lalrpop-util = "0.19.8"
regex = "1"

To this:

[package]
name = "calculator"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[build-dependencies] # <-- We added this and everything after!
lalrpop = "0.19.8"

[dependencies]
lalrpop = "0.19.8"
lalrpop-util = "0.19.8"
regex = "1"

As far as I can tell that's what needs to happen, but if there's a something better to do I'd be happy to hear about it.

@aristotaloss
Copy link

lalrpop-util = { version = "0.19.7", features = ["lexer"] }

I think that'll do the trick. I think they may have forgotten to include it in the docs after this commit:
e2310c8

@nikomatsakis nikomatsakis added this to the 1.0 milestone Mar 21, 2023
@yannham
Copy link
Contributor

yannham commented Apr 11, 2023

Hi @alanwsmith, thanks a lot for reporting. I can confirm what @Velocity- says, which sounds both reasonable and indeed fixes the issue in practice, without needing an additional non build-deps toward LALRPOP. If you're willing to implement this change on this PR (or open a new one), I'm willing to review and merge !

@nikomatsakis
Copy link
Collaborator

hmm, I wonder if lexer should be a default feature -- separate question though

Copy link
Contributor

@yannham yannham left a comment

Choose a reason for hiding this comment

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

This looks ok to me. The lexer feature of LALRPOP just enables the same feature in lalrpop-util, so moving the feature to lalrpop-util should do exactly the same, while being more accurate IMHO (in the end, the crate exposing the feature is lalrpop-util)

@alanwsmith
Copy link
Contributor Author

Hi @alanwsmith, thanks a lot for reporting. I can confirm what @Velocity- says, which sounds both reasonable and indeed fixes the issue in practice, without needing an additional non build-deps toward LALRPOP. If you're willing to implement this change on this PR (or open a new one), I'm willing to review and merge !

I'm afraid I don't know my way around pull requests very well. It sounds like maybe it already got fixed and I can just close this?

alanwsmith and others added 2 commits July 3, 2023 13:01
Co-Authored-By: Niko Matsakis <niko@alum.mit.edu>
@yannham
Copy link
Contributor

yannham commented Jul 3, 2023

Nothing much is left for this PR, as the lexer feature has been added in separate PRs. There is still the README fixup though, that could be useful to merge. I've rebased it. @youknowone could you give a quick look?

@yannham yannham requested a review from youknowone July 3, 2023 11:09
@yannham yannham requested review from nikomatsakis and removed request for nikomatsakis July 3, 2023 14:27
@yannham yannham merged commit 2f2bd48 into lalrpop:master Jul 3, 2023
benjaminfjones added a commit to benjaminfjones/presburger that referenced this pull request Sep 1, 2023
Update how lalropop is declared in Cargo.toml to work around
lalrpop/lalrpop#711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants