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

Fix ICE in decimal_literal_representation lint #3931

Merged
merged 4 commits into from Apr 10, 2019
Merged

Conversation

@phansch
Copy link
Member

@phansch phansch commented Apr 8, 2019

Handling the integer parsing properly instead of just unwrapping.

Note that the test is not catching the ICE because plain UI tests
currently hide ICEs. Once that issue is fixed, this
test would fail properly again.

Fixes #3891

Handling the integer parsing properly instead of just unwrapping.

Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.

[compiletest_issue]: Manishearth/compiletest-rs#169
@phansch
Copy link
Member Author

@phansch phansch commented Apr 8, 2019

For some reason rustfmt is triggering the same rust warning: https://travis-ci.com/rust-lang/rust-clippy/jobs/191281098#L1664
I didn't know that running rustfmt will trigger rustc errors as well and I'm not sure how to properly test this then? #![rustfmt::skip] has no effect either.

@flip1995
Copy link
Member

@flip1995 flip1995 commented Apr 9, 2019

Maybe use the old hack and exclude the test file here:

for file in `find tests | grep "\.rs$"` ; do

rustfmt only triggers parsing errors, since the file needs to be parsed for rustfmt to run.

Because the code triggers a rustc parse error which makes rustfmt fail.
@phansch
Copy link
Member Author

@phansch phansch commented Apr 9, 2019

I re-added the old hack to exclude the file from rustfmt. I think in this specific case it makes sense to be excluded.

clippy_lints/src/literal_representation.rs Outdated Show resolved Hide resolved
clippy_lints/src/literal_representation.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

@flip1995 flip1995 commented Apr 10, 2019

One last thing: All these if statements should be collapsible into the if_chain! macro:

if_chain! {
    if let LitKind::Int(..) = lit.node;
    if let Some(src) = snippet_opt(cx, lit.span);
    if let Some(firstch) = src.chars().next();
    if char::to_digit(firstch, 10).is_some();
    let digit_info = DigitInfo::new(&src, false);
    if digit_info.radix == Radix::Decimal;
    if let Ok(val) = digit_info.digits;
    if val >= u128::from(self.threshold);
    then {
        let hex = format!("{:#X}", val);
        let digit_info = DigitInfo::new(&hex, false);
        let _ = Self::do_lint(digit_info.digits).map_err(|warning_type| {
            warning_type.display(&digit_info.grouping_hint(), cx, lit.span)
        });
    }
}

(For some reason I wasn't able to comment directly on the code)

@phansch
Copy link
Member Author

@phansch phansch commented Apr 10, 2019

Yup, that worked =)

@flip1995
Copy link
Member

@flip1995 flip1995 commented Apr 10, 2019

@bors r+

@bors
Copy link
Contributor

@bors bors commented Apr 10, 2019

📌 Commit ab6b949 has been approved by flip1995

bors added a commit that referenced this issue Apr 10, 2019
Fix ICE in decimal_literal_representation lint

Handling the integer parsing properly instead of just unwrapping.

Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.

Fixes #3891

[compiletest_issue]: Manishearth/compiletest-rs#169
@bors
Copy link
Contributor

@bors bors commented Apr 10, 2019

Testing commit ab6b949 with merge 2278814...

@bors
Copy link
Contributor

@bors bors commented Apr 10, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 2278814 to master...

@bors bors merged commit ab6b949 into rust-lang:master Apr 10, 2019
3 checks passed
3 checks passed
@travis-ci[bot]
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@bors
homu Test successful
Details
@phansch phansch deleted the 3891 branch Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants