Skip to content

Commit

Permalink
Keep a single SourceMap in DiagnosticList, not two
Browse files Browse the repository at this point in the history
Align with upcoming changes in #747
  • Loading branch information
SimonSapin committed Nov 23, 2023
1 parent 8c00ce8 commit 18bd1a6
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 53 deletions.
4 changes: 2 additions & 2 deletions crates/apollo-compiler/src/ast/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ impl Document {
///
/// Does not perform any validation beyond this syntactic level.
pub fn check_parse_errors(&self) -> Result<(), DiagnosticList> {
let mut errors = DiagnosticList::new(None, self.sources.clone());
let mut errors = DiagnosticList::new(self.sources.clone());
for (file_id, source) in self.sources.iter() {
source.validate_parse_errors(&mut errors, *file_id)
}
Expand All @@ -54,7 +54,7 @@ impl Document {
self,
type_system_definitions_are_errors,
);
let mut errors = DiagnosticList::new(None, self.sources.clone());
let mut errors = DiagnosticList::new(self.sources.clone());
crate::executable::validation::validate_standalone_executable(&mut errors, &executable);
errors.into_result()
}
Expand Down
9 changes: 7 additions & 2 deletions crates/apollo-compiler/src/executable/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub use crate::ast::{
use crate::validation::DiagnosticList;
use crate::validation::NodeLocation;
use std::fmt;
use std::sync::Arc;

/// Executable definitions, annotated with type information
#[derive(Debug, Clone, Default)]
Expand Down Expand Up @@ -228,7 +229,9 @@ impl ExecutableDocument {
}

pub fn validate(&self, schema: &Schema) -> Result<(), DiagnosticList> {
let mut errors = DiagnosticList::new(Some(schema.sources.clone()), self.sources.clone());
let mut sources = IndexMap::clone(&schema.sources);
sources.extend(self.sources.iter().map(|(k, v)| (*k, v.clone())));
let mut errors = DiagnosticList::new(Arc::new(sources));
validation::validate_executable_document(&mut errors, schema, self);
errors.into_result()
}
Expand Down Expand Up @@ -677,7 +680,9 @@ impl FieldSet {
}

pub fn validate(&self, schema: &Schema) -> Result<(), DiagnosticList> {
let mut errors = DiagnosticList::new(Some(schema.sources.clone()), self.sources.clone());
let mut sources = IndexMap::clone(&schema.sources);
sources.extend(self.sources.iter().map(|(k, v)| (*k, v.clone())));
let mut errors = DiagnosticList::new(Arc::new(sources));
validation::validate_field_set(&mut errors, schema, self);
errors.into_result()
}
Expand Down
2 changes: 1 addition & 1 deletion crates/apollo-compiler/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ impl Parser {
let file_id = FileId::new();

let sources: crate::SourceMap = Arc::new([(file_id, source_file)].into());
let mut errors = DiagnosticList::new(None, sources.clone());
let mut errors = DiagnosticList::new(sources.clone());
for (file_id, source) in sources.iter() {
source.validate_parse_errors(&mut errors, *file_id)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/apollo-compiler/src/schema/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ impl Schema {

/// Returns `Err` if invalid
pub fn validate(&self) -> Result<(), DiagnosticList> {
let mut errors = DiagnosticList::new(None, self.sources.clone());
let mut errors = DiagnosticList::new(self.sources.clone());
validation::validate_schema(&mut errors, self);
errors.into_result()
}
Expand Down
70 changes: 23 additions & 47 deletions crates/apollo-compiler/src/validation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,19 @@ pub use crate::database::FileId;
pub use crate::node::NodeLocation;

/// A collection of diagnostics returned by some validation method
pub struct DiagnosticList(Box<DiagnosticListBoxed>);

/// Box indirection to avoid large `Err` values:
/// <https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err>
struct DiagnosticListBoxed {
sources: Sources,
pub struct DiagnosticList {
sources: SourceMap,
diagnostics_data: Vec<DiagnosticData>,
}

struct Sources {
schema_sources: Option<SourceMap>,
self_sources: SourceMap,
}

struct DiagnosticData {
location: Option<NodeLocation>,
details: Details,
}

/// A single diagnostic in a [`DiagnosticList`]
pub struct Diagnostic<'a> {
sources: &'a Sources,
sources: &'a SourceMap,
data: &'a DiagnosticData,
}

Expand Down Expand Up @@ -300,19 +291,26 @@ impl<'a> Diagnostic<'a> {
}

impl DiagnosticList {
pub(crate) fn new(sources: SourceMap) -> Self {
Self {
sources,
diagnostics_data: Vec::new(),
}
}

pub fn is_empty(&self) -> bool {
self.0.diagnostics_data.is_empty()
self.diagnostics_data.is_empty()
}

pub fn len(&self) -> usize {
self.0.diagnostics_data.len()
self.diagnostics_data.len()
}

pub fn iter(
&self,
) -> impl Iterator<Item = Diagnostic<'_>> + DoubleEndedIterator + ExactSizeIterator {
self.0.diagnostics_data.iter().map(|data| Diagnostic {
sources: &self.0.sources,
self.diagnostics_data.iter().map(|data| Diagnostic {
sources: &self.sources,
data,
})
}
Expand All @@ -325,36 +323,20 @@ impl DiagnosticList {
format!("{self:#}")
}

pub(crate) fn new(schema_sources: Option<SourceMap>, self_sources: SourceMap) -> Self {
Self(Box::new(DiagnosticListBoxed {
sources: Sources {
schema_sources,
self_sources,
},
diagnostics_data: Vec::new(),
}))
}

pub(crate) fn push(&mut self, location: Option<NodeLocation>, details: Details) {
self.0
.diagnostics_data
self.diagnostics_data
.push(DiagnosticData { location, details })
}

pub(crate) fn into_result(mut self) -> Result<(), Self> {
if self.0.diagnostics_data.is_empty() {
if self.diagnostics_data.is_empty() {
Ok(())
} else {
self.sort();
self.diagnostics_data
.sort_by_key(|err| err.location.map(|loc| (loc.file_id(), loc.offset())));
Err(self)
}
}

pub(crate) fn sort(&mut self) {
self.0
.diagnostics_data
.sort_by_key(|err| err.location.map(|loc| (loc.file_id(), loc.offset())))
}
}

/// Defaults to ANSI color codes if stderr is a terminal.
Expand Down Expand Up @@ -391,28 +373,22 @@ impl fmt::Display for Diagnostic<'_> {
let color = !f.alternate();
self.data
.report(color)
.write(self.sources, Adaptor(f))
.write(Cache(self.sources), Adaptor(f))
.map_err(|_| fmt::Error)
}
}

impl Sources {
pub(crate) fn get(&self, file_id: &FileId) -> Option<&Arc<SourceFile>> {
self.self_sources
.get(file_id)
.or_else(|| self.schema_sources.as_ref()?.get(file_id))
}
}
struct Cache<'a>(&'a SourceMap);

impl ariadne::Cache<FileId> for &'_ Sources {
impl ariadne::Cache<FileId> for Cache<'_> {
fn fetch(&mut self, file_id: &FileId) -> Result<&ariadne::Source, Box<dyn fmt::Debug + '_>> {
struct NotFound(FileId);
impl fmt::Debug for NotFound {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "source file not found: {:?}", self.0)
}
}
if let Some(source_file) = self.get(file_id) {
if let Some(source_file) = self.0.get(file_id) {
Ok(source_file.ariadne())
} else if *file_id == FileId::NONE || *file_id == FileId::HACK_TMP {
static EMPTY: OnceLock<ariadne::Source> = OnceLock::new();
Expand All @@ -430,7 +406,7 @@ impl ariadne::Cache<FileId> for &'_ Sources {
self.0.path().display().fmt(f)
}
}
let source_file = self.get(file_id)?;
let source_file = self.0.get(file_id)?;
Some(Box::new(Path(source_file.clone())))
} else {
struct NoSourceFile;
Expand Down

0 comments on commit 18bd1a6

Please sign in to comment.