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

EmitterWriter::get_max_line_num works incorrectly #64447

Closed
AnthonyMikh opened this issue Sep 14, 2019 · 2 comments
Closed

EmitterWriter::get_max_line_num works incorrectly #64447

AnthonyMikh opened this issue Sep 14, 2019 · 2 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@AnthonyMikh
Copy link
Contributor

AnthonyMikh commented Sep 14, 2019

TL;DR: EmitterWriter::get_max_line_num unconditionally returns self.get_multispan_max_line_num(span) no matter what is contained in children.

Longer explanation: let's take a look at the code of method (it's rather short):

    fn get_max_line_num(&mut self, span: &MultiSpan, children: &[SubDiagnostic]) -> usize {
        let mut max = 0;

        let primary = self.get_multispan_max_line_num(span);
        max = if primary > max { primary } else { max };

        for sub in children {
            let sub_result = self.get_multispan_max_line_num(&sub.span);
            max = if sub_result > max { primary } else { max };
        }
        max
    }

Here self.get_multispan_max_line_num(span) returns a plain usize. Firstly, since 0 is the smallest possible value of usize, the first three lines can be rewritten without changing the meaning as

let primary = self.get_multispan_max_line_num(span);
let mut max = primary;

So after executing these lines max == primary. Secondly, in the loop if compares sub_result with max but assigns either max or primary. If max == primary in the beginning of iteration, then this also holds in the end of iteration. Since this preposition holds before executing the loop, it also holds after executing the loop, so the whole method just returns primary, QED.

Erroneous code was introduced in 71ec286.

Unfortunately, I'm not familliar enough with the code to propose the correct fix.

@Centril Centril added A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 14, 2019
@Centril
Copy link
Contributor

Centril commented Sep 14, 2019

cc @estebank @phansch

hman523 added a commit to hman523/rust that referenced this issue Sep 23, 2019
Centril added a commit to Centril/rust that referenced this issue Sep 24, 2019
Fixed issue from rust-lang#64447

Did two tiny fixes. One is a micro optimization since we know that max is going to be assigned a `usize`, we do not have to worry about a possible negative number.
The other issue that was fixed is that the max from the children isn't updated correctly. Now it will use `sub_result` instead of `primary` and will properly get the needed value.
bors added a commit that referenced this issue Sep 24, 2019
Rollup of 16 pull requests

Successful merges:

 - #63356 (Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example)
 - #63934 (Fix coherence checking for impl trait in type aliases)
 - #64016 (Streamline `Compiler`)
 - #64296 (Document the unstable iter_order_by library feature)
 - #64443 (rustdoc: general cleanup)
 - #64622 (Add a cycle detector for generic `Graph`s and `mir::Body`s)
 - #64689 (Refactor macro by example)
 - #64698 (Recover on `const X = 42;` and infer type + Error Stash API)
 - #64702 (Remove unused dependencies)
 - #64717 (update mem::discriminant test to use assert_eq and assert_ne over comparison operators)
 - #64720 ( remove rtp.rs, and move rtpSpawn and RTP_ID_ERROR to libc)
 - #64721 (Fixed issue from #64447)
 - #64725 (fix one typo)
 - #64737 (fix several issues in String docs)
 - #64742 (relnotes: make compatibility section more sterile and fix rustc version)
 - #64748 (Fix #64744. Account for the Zero sub-pattern case.)

Failed merges:

r? @ghost
@AnthonyMikh
Copy link
Contributor Author

Closing as fixed by #64721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants