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

Add message-format=json support, second take #64

Merged
merged 13 commits into from
Jan 24, 2019

Conversation

konstin
Copy link
Contributor

@konstin konstin commented Jan 22, 2019

This is #53 with the merge conflicts resolved and the review comments addressed. I also copied DiagnosticLevel from @epage.

CC @roblabla

@konstin
Copy link
Contributor Author

konstin commented Jan 22, 2019

There's one clippy warning but I'm not sure what to do about that since the memory layout of that enum should not be an issue at all.

warning: large size difference between variants
  --> src/messages.rs:86:5
   |
86 |     CompilerMessage(FromCompiler),
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: #[warn(clippy::large_enum_variant)] on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
   |
86 |     CompilerMessage(Box<FromCompiler>),
   |                     ^^^^^^^^^^^^^^^^^

@roblabla
Copy link
Contributor

Thanks for doing this. I've been meaning to do it but haven't been able to find the time 😓 .

The vast majority of messages from cargo will be of the FromCompiler, so I don't think forcing one more allocation in that path is a good idea, especially when the gains are fairly trivial. I feel like the warning should be ignored in this case.

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Just some minor documentation notes.

/// "error: internal compiler error", "error", "warning", "note", "help"
pub level: DiagnosticLevel,
/// A list of source code spans this diagnostic is associated with.
pub spans: Vec<DiagnosticSpan>,
Copy link
Contributor

@ehuss ehuss Jan 23, 2019

Choose a reason for hiding this comment

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

Maybe add a note that this may be empty? Some messages don't have a span (like "main not found"), and child messages sometimes don't have spans when they are attached to their parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's clear that that Vec can be empty

pub spans: Vec<DiagnosticSpan>,
/// Associated diagnostic messages.
pub children: Vec<Diagnostic>,
/// The message as rustc would render it
Copy link
Contributor

Choose a reason for hiding this comment

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

This changed in 1.23. Before 1.23 it was used by suggested replacements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we just assume that everyone uses a reasonably recent compiler version?

src/diagnostic.rs Outdated Show resolved Hide resolved
src/diagnostic.rs Show resolved Hide resolved
src/messages.rs Outdated Show resolved Hide resolved
src/messages.rs Outdated Show resolved Hide resolved
src/messages.rs Outdated Show resolved Hide resolved
src/messages.rs Outdated Show resolved Hide resolved
src/messages.rs Outdated Show resolved Hide resolved
@konstin
Copy link
Contributor Author

konstin commented Jan 24, 2019

Thanks for the review @ehuss, I've fixed most doc strings now.

Copy link
Owner

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

The cargo team would like to have the cargo_diagnostics crate under their organization. I think it's fine to start out in a private repo and let them fork or move it

src/dependency.rs Show resolved Hide resolved
@@ -0,0 +1,149 @@
//! This module contains `Diagnostic` and the types/functions it uses for deserialization.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you put these types into their own crate so we can reuse them from the compiler and from rustfix? (maybe rustc_diagnostics)

/// Profile settings used to determine which compiler flags to use for a
/// target.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct ArtifactProfile {
Copy link
Owner

Choose a reason for hiding this comment

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

It would make sense to share these types with cargo. cc #63

We could have a dep graph like
grafik

where each crate reexports its dependencies

@konstin
Copy link
Contributor Author

konstin commented Jan 24, 2019

I'd love to have a base crate shared with cargo. Imo we should track that in one central issue so information doesn't get lost across thread, maybe directly in the cargo repo.

Personally, I'd like to have some kind of cargo_shared_types crate that contains types for all the json cargo emits, which is currently diagnostics, cargo metadata and the build plan. But for now I'd like to have this merged and released before doing bigger refactorings work because I'm already using this branch in pyo3-pack.

@oli-obk
Copy link
Owner

oli-obk commented Jan 24, 2019

The cargo team would like to have the cargo_diagnostics crate under their organization. I think it's fine to start out in a private repo and let them fork or move it

Actually they just want it to be a subcrate of the cargo repo. Can you make a crate out of each of the two modules and add them to the cargo repo?

@oli-obk
Copy link
Owner

oli-obk commented Jan 24, 2019

Personally, I'd like to have some kind of cargo_shared_types crate that contains types for all the json cargo emits, which is currently diagnostics

We should still split it in two as I suggested above, because other crates (e.g. rustc) won't need to know about the cargo_diagnostics, just about the rustc_diagnostics

But for now I'd like to have this merged and released before doing bigger refactorings work because I'm already using this branch in pyo3-pack.

I guess it won't even be a breaking change if we just make it pub use rustc_diagnostics as diagnostics; So seems ok to me

@oli-obk oli-obk merged commit 85ecce7 into oli-obk:master Jan 24, 2019
@konstin konstin deleted the parse_messages branch January 24, 2019 21:43
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.

4 participants