Skip to content
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

Teach compiletest about multispans #31380

Closed
mitaa opened this issue Feb 3, 2016 · 8 comments
Closed

Teach compiletest about multispans #31380

mitaa opened this issue Feb 3, 2016 · 8 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-feature-request Category: A feature request, i.e: not implemented / a PR. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@mitaa
Copy link
Contributor

mitaa commented Feb 3, 2016

Compiletest needs to understand multispans before they can actually be used in compiler output.

This makes expected-error-checking more complicated because multiple errors can be folded into a single one, breaking a core assumption of the current code.

Because of this I suggest to switch to the new JSON error output format. (though how should stderr, raw json, be shown to the user in case of test failure?)

cc @nrc, @brson

@nrc
Copy link
Member

nrc commented Feb 3, 2016

I don't think switching to JSON helps - you still have the same problem with multiple spans.

@mitaa
Copy link
Contributor Author

mitaa commented Feb 3, 2016

I guess you're right. That was more of a general suggestion/question to use machine-readable output for machine related stuff, instead of ad-hoc parsing.

edit: (should I open another issue for that?)

@brson
Copy link
Contributor

brson commented Feb 5, 2016

I don't know enough about multispans to know what the new requirements are. @nikomatsakis probably did most of the error parsing in compiletest.

I would be reluctant to switch to machine readable output unless we're super confident that the console output is formatted correctly and consistently whenever the machine output is correct. Probably worth a different issue, yes.

@nikomatsakis
Copy link
Contributor

Sorry, what is a multispan? :)

@nrc
Copy link
Member

nrc commented Feb 10, 2016

@nikomatsakis a MultiSpan is like a Span, but covers multiple disjoint spans.

@nikomatsakis
Copy link
Contributor

On Wed, Feb 10, 2016 at 12:29:07PM -0800, Nick Cameron wrote:

@nikomatsakis a MultiSpan is like a Span, but covers multiple disjoint spans.

I kind of guessed that :) but can someone give an example of an error that
uses one and which becomes more complicated now?

@mitaa
Copy link
Contributor Author

mitaa commented Mar 17, 2016

The only concrete planned use case is the unused import lint.

For example, compile-fail/lint-unused-imports.rs has this line:

// Should get errors for both 'Some' and 'None'
use std::option::Option::{Some, None}; //~ ERROR unused import
//~^ ERROR unused import

This should be reported, using MultiSpans, as

foo.rs:20:27: 20:31 error: unused import(s)
foo.rs:20 use std::option::Option::{Some, None}; //~ ERROR unused import
                                    ^~~~  ^~~~

AIUI compiletest counts each emitted error message and parses the line number following the filename.
This is now a single error message and thus a single error according to compiletest, but how do we make sure that the error is reported for both imports?

I think this would require to track emitted spans somewhat - but the count of the emitted spans isn't necessarily equal to the expected error count (e.g. this suggestion).

edit:
perhaps just an option to specify the expected span count of an error message?

// Should get errors for both 'Some' and 'None'
use std::option::Option::{Some, None}; //~ ERROR^2 unused import

@steveklabnik steveklabnik added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed A-compiler labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jun 20, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 24, 2017
@Mark-Simulacrum
Copy link
Member

I feel like this has been done since this issue was opened; if we need this still though feel free to comment here and we can reopen!

@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-feature-request Category: A feature request, i.e: not implemented / a PR. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants