Skip to content

#[cfg(version)] ignores invalid version literal #79436

New issue

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

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

Already on GitHub? # to your account

Closed
taiki-e opened this issue Nov 26, 2020 · 3 comments · Fixed by #81259
Closed

#[cfg(version)] ignores invalid version literal #79436

taiki-e opened this issue Nov 26, 2020 · 3 comments · Fixed by #81259
Labels
C-bug Category: This is a bug. F-cfg_version `#![feature(cfg_version)]` requires-nightly This issue requires a nightly compiler in some way.

Comments

@taiki-e
Copy link
Member

taiki-e commented Nov 26, 2020

I tried this code:

#![feature(cfg_version)]

#[cfg(version("aa.0"))]
mod foo {}

I expected to see this happen: error or warning about invalid version literal

Instead, this happened: no error or warning
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=3f14787508e791c693ac85bf41973b97

Mentioning @mibac138 who implemented #[cfg(version)] in #71314.

@taiki-e taiki-e added the C-bug Category: This is a bug. label Nov 26, 2020
@taiki-e
Copy link
Member Author

taiki-e commented Nov 26, 2020

It seems that this feature is currently using an external crate for version parsing (and looks like this issue is a bug of that crate?), but IIUC, what this feature really needs is a simple parser like this, right?

// based on https://github.com/cuviper/autocfg/blob/1.0.1/src/version.rs#L7
// can be compare by using `>=`: https://github.com/cuviper/autocfg/blob/1.0.1/src/lib.rs#L218
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
struct Version {
    major: usize,
    minor: usize,
    patch: usize,
}

fn parse_version(s: &str) -> Result<Version, Box<dyn std::error::Error + Send + Sync>> {
    let mut digits = s.splitn(3, '.'); // Maybe we can pass a string that is split into three from the beginning?
    let major = digits.next().ok_or("missing major version")?.parse()?; // using `.trim()` before `.parse()` is may be prefer? https://github.com/rust-lang/rust/issues/64796#issuecomment-622984135
    let minor = digits.next().ok_or("missing minor version")?.parse()?;
    let patch = digits.next().unwrap_or("0").parse()?;
    Ok(Version {
        major,
        minor,
        patch,
    })
}

If so, it may be preferable to drop the dependency and use our own parser. (looks like that crate also has some other issues: #64796 (comment))

@petrochenkov
Copy link
Contributor

I agree that we need a custom simplified but fully controlled version parser for this instead of relying on version_check.

@jonas-schievink jonas-schievink added F-cfg_version `#![feature(cfg_version)]` requires-nightly This issue requires a nightly compiler in some way. labels Nov 26, 2020
@est31
Copy link
Member

est31 commented Jan 22, 2021

Filed a PR: #81259

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: This is a bug. F-cfg_version `#![feature(cfg_version)]` requires-nightly This issue requires a nightly compiler in some way.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants