Skip to content

Commit 1e8042a

Browse files
rbalicki2rbalicki2
committed
introduce WithLocationForDisplay struct
- We run into bugs where we originally have a field of type T, which changes to a field of type WithLocation<T>. Both implement Display, so we end up printing the wrong thing. - There are no automated tests, usually, that will catch this. - Now, WithLocation no longer implements Display. Instead, you must call for_display() and print that.
1 parent 52ebd27 commit 1e8042a

File tree

7 files changed

+92
-48
lines changed

7 files changed

+92
-48
lines changed

‎crates/common_lang_types/src/location.rs‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,16 +140,10 @@ pub struct WithLocation<T> {
140140
pub item: T,
141141
}
142142

143-
impl<T: Error> Error for WithLocation<T> {
143+
impl<T: Error> Error for WithLocationForDisplay<'_, T> {
144144
fn description(&self) -> &str {
145145
#[allow(deprecated)]
146-
self.item.description()
147-
}
148-
}
149-
150-
impl<T: fmt::Display> fmt::Display for WithLocation<T> {
151-
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
152-
write!(f, "{}\n{}", self.item, self.location)
146+
self.inner.item.description()
153147
}
154148
}
155149

@@ -180,6 +174,25 @@ impl<T> WithLocation<T> {
180174
span,
181175
}
182176
}
177+
178+
pub fn for_display(&self) -> WithLocationForDisplay<'_, T> {
179+
WithLocationForDisplay { inner: self }
180+
}
181+
}
182+
183+
/// [WithLocation] does not implement Display. There have been a few bugs in which
184+
/// a field was initially T, and then changed to be WithLocation<T>, and as a result,
185+
/// what was printed changed unexpectedly. So, we require an explicit step to print
186+
/// a WithLocation, calling with_location.for_display().
187+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
188+
pub struct WithLocationForDisplay<'a, T> {
189+
inner: &'a WithLocation<T>,
190+
}
191+
192+
impl<T: fmt::Display> fmt::Display for WithLocationForDisplay<'_, T> {
193+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
194+
write!(f, "{}\n{}", self.inner.item, self.inner.location)
195+
}
183196
}
184197

185198
#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq, Ord, PartialOrd)]

‎crates/graphql_network_protocol/src/graphql_network_protocol.rs‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,15 +244,13 @@ impl GraphQLSchemaOriginalDefinitionType {
244244

245245
#[derive(Error, Debug, Clone, PartialEq, Eq)]
246246
pub enum ParseAndProcessGraphQLTypeSystemDocumentsError {
247-
#[error("{message}")]
247+
#[error("{}", message.for_display())]
248248
SchemaParse {
249-
#[from]
250249
message: WithLocation<SchemaParseError>,
251250
},
252251

253-
#[error("{message}")]
252+
#[error("{}", message.for_display())]
254253
ProcessGraphQLTypeSystemDefinitionWithLocation {
255-
#[from]
256254
message: WithLocation<ProcessGraphqlTypeSystemDefinitionError>,
257255
},
258256

@@ -262,9 +260,30 @@ pub enum ParseAndProcessGraphQLTypeSystemDocumentsError {
262260
message: ProcessGraphqlTypeSystemDefinitionError,
263261
},
264262

265-
#[error("{message}")]
263+
#[error("{}", message.for_display())]
266264
CreateAdditionalFields {
267-
#[from]
268265
message: WithLocation<CreateAdditionalFieldsError>,
269266
},
270267
}
268+
269+
impl From<WithLocation<SchemaParseError>> for ParseAndProcessGraphQLTypeSystemDocumentsError {
270+
fn from(value: WithLocation<SchemaParseError>) -> Self {
271+
Self::SchemaParse { message: value }
272+
}
273+
}
274+
275+
impl From<WithLocation<ProcessGraphqlTypeSystemDefinitionError>>
276+
for ParseAndProcessGraphQLTypeSystemDocumentsError
277+
{
278+
fn from(value: WithLocation<ProcessGraphqlTypeSystemDefinitionError>) -> Self {
279+
Self::ProcessGraphQLTypeSystemDefinitionWithLocation { message: value }
280+
}
281+
}
282+
283+
impl From<WithLocation<CreateAdditionalFieldsError>>
284+
for ParseAndProcessGraphQLTypeSystemDocumentsError
285+
{
286+
fn from(value: WithLocation<CreateAdditionalFieldsError>) -> Self {
287+
Self::CreateAdditionalFields { message: value }
288+
}
289+
}

‎crates/isograph_compiler/src/add_selection_sets.rs‎

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -260,14 +260,14 @@ fn get_validated_scalar_selection<TNetworkProtocol: NetworkProtocol + 'static>(
260260
)
261261
.expect(
262262
"Expected selectable to exist. \
263-
This is indicative of a bug in Isograph.",
263+
This is indicative of a bug in Isograph.",
264264
);
265265
let object = schema
266266
.server_entity_data
267267
.server_object_entity(*object_selectable.target_object_entity.inner())
268268
.expect(
269269
"Expected entity to exist. \
270-
This is indicative of a bug in Isograph.",
270+
This is indicative of a bug in Isograph.",
271271
);
272272

273273
WithLocation::new(
@@ -294,16 +294,16 @@ fn get_validated_scalar_selection<TNetworkProtocol: NetworkProtocol + 'static>(
294294
*client_type.as_scalar().as_ref().ok_or_else(|| {
295295
WithLocation::new(
296296
AddSelectionSetsError::SelectionTypeSelectionClientPointerSelectedAsScalar {
297-
client_field_parent_type_name: top_level_field_or_pointer
298-
.type_and_field()
299-
.type_name,
300-
client_field_name: top_level_field_or_pointer.type_and_field().field_name,
301-
field_parent_type_name: selection_parent_object.name.item,
302-
field_name: scalar_selection.name.item.into(),
303-
client_type: top_level_field_or_pointer.client_type().to_string(),
304-
},
305-
scalar_selection.name.location,
306-
)
297+
client_field_parent_type_name: top_level_field_or_pointer
298+
.type_and_field()
299+
.type_name,
300+
client_field_name: top_level_field_or_pointer.type_and_field().field_name,
301+
field_parent_type_name: selection_parent_object.name.item,
302+
field_name: scalar_selection.name.item.into(),
303+
client_type: top_level_field_or_pointer.client_type().to_string(),
304+
},
305+
scalar_selection.name.location,
306+
)
307307
})?;
308308
DefinitionLocation::Client((parent_object_entity_name, client_field_name))
309309
}
@@ -408,17 +408,17 @@ fn get_validated_object_selection<TNetworkProtocol: NetworkProtocol + 'static>(
408408
let (parent_object_entity_name, client_pointer_name) =
409409
*client_type.as_object().as_ref().ok_or_else(|| {
410410
vec![WithLocation::new(
411-
AddSelectionSetsError::SelectionTypeSelectionClientPointerSelectedAsScalar {
412-
client_field_parent_type_name: top_level_field_or_pointer
413-
.type_and_field()
414-
.type_name,
415-
client_field_name: top_level_field_or_pointer.type_and_field().field_name,
416-
field_parent_type_name: selection_parent_object.name.item,
417-
field_name: object_selection.name.item.into(),
418-
client_type: top_level_field_or_pointer.client_type().to_string(),
419-
},
420-
Location::generated(),
421-
)]
411+
AddSelectionSetsError::SelectionTypeSelectionClientPointerSelectedAsScalar {
412+
client_field_parent_type_name: top_level_field_or_pointer
413+
.type_and_field()
414+
.type_name,
415+
client_field_name: top_level_field_or_pointer.type_and_field().field_name,
416+
field_parent_type_name: selection_parent_object.name.item,
417+
field_name: object_selection.name.item.into(),
418+
client_type: top_level_field_or_pointer.client_type().to_string(),
419+
},
420+
Location::generated(),
421+
)]
422422
})?;
423423
let client_pointer = schema
424424
.client_pointer(parent_object_entity_name, client_pointer_name)

‎crates/isograph_compiler/src/create_schema.rs‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,24 +132,23 @@ pub enum ProcessIsoLiteralsForSchemaError {
132132
"{}{}",
133133
if messages.len() == 1 { "Unable to process Isograph literal:" } else { "Unable to process Isograph literals:" },
134134
messages.iter().fold(String::new(), |mut output, x| {
135-
output.push_str(&format!("\n\n{x}"));
135+
output.push_str(&format!("\n\n{}", x.for_display()));
136136
output
137137
})
138138
)]
139139
ProcessIsoLiterals {
140140
messages: Vec<WithLocation<ProcessClientFieldDeclarationError>>,
141141
},
142142

143-
#[error("{error}")]
143+
#[error("{}", error.for_display())]
144144
ProcessTypeDefinition {
145-
#[from]
146145
error: WithLocation<CreateAdditionalFieldsError>,
147146
},
148147

149148
#[error(
150149
"{}",
151150
messages.iter().fold(String::new(), |mut output, x| {
152-
output.push_str(&format!("\n\n{x}"));
151+
output.push_str(&format!("\n\n{}", x.for_display()));
153152
output
154153
})
155154
)]
@@ -160,7 +159,7 @@ pub enum ProcessIsoLiteralsForSchemaError {
160159
#[error(
161160
"{}",
162161
messages.iter().fold(String::new(), |mut output, x| {
163-
output.push_str(&format!("\n\n{x}"));
162+
output.push_str(&format!("\n\n{}", x.for_display()));
164163
output
165164
})
166165
)]
@@ -171,7 +170,7 @@ pub enum ProcessIsoLiteralsForSchemaError {
171170
#[error(
172171
"{}",
173172
messages.iter().fold(String::new(), |mut output, x| {
174-
output.push_str(&format!("\n\n{x}"));
173+
output.push_str(&format!("\n\n{}", x.for_display()));
175174
output
176175
})
177176
)]
@@ -208,6 +207,12 @@ impl From<Vec<WithLocation<AddSelectionSetsError>>> for ProcessIsoLiteralsForSch
208207
}
209208
}
210209

210+
impl From<WithLocation<CreateAdditionalFieldsError>> for ProcessIsoLiteralsForSchemaError {
211+
fn from(value: WithLocation<CreateAdditionalFieldsError>) -> Self {
212+
ProcessIsoLiteralsForSchemaError::ProcessTypeDefinition { error: value }
213+
}
214+
}
215+
211216
fn parse_iso_literals<TNetworkProtocol: NetworkProtocol + 'static>(
212217
db: &IsographDatabase<TNetworkProtocol>,
213218
) -> Result<ParsedIsoLiteralsMap, Vec<WithLocation<IsographLiteralParseError>>> {
@@ -500,9 +505,16 @@ pub enum CreateSchemaError<TNetworkProtocol: NetworkProtocol + 'static> {
500505
message: TNetworkProtocol::ParseAndProcessTypeSystemDocumentsError,
501506
},
502507

503-
#[error("{message}")]
508+
#[error("{}", message.for_display())]
504509
CreateAdditionalFields {
505-
#[from]
506510
message: WithLocation<CreateAdditionalFieldsError>,
507511
},
508512
}
513+
514+
impl<TNetworkProtocol: NetworkProtocol> From<WithLocation<CreateAdditionalFieldsError>>
515+
for CreateSchemaError<TNetworkProtocol>
516+
{
517+
fn from(value: WithLocation<CreateAdditionalFieldsError>) -> Self {
518+
CreateSchemaError::CreateAdditionalFields { message: value }
519+
}
520+
}

‎crates/isograph_compiler/src/get_validated_schema.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ pub enum GetValidatedSchemaError<TNetworkProtocol: NetworkProtocol + 'static> {
3131
#[error(
3232
"{}",
3333
messages.iter().fold(String::new(), |mut output, x| {
34-
output.push_str(&format!("\n\n{x}"));
34+
output.push_str(&format!("\n\n{}", x.for_display()));
3535
output
3636
})
3737
)]

‎crates/isograph_fixture_tests/src/main.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ fn generate_content_for_output_file(
147147
Err(errs) => {
148148
let mut s = String::new();
149149
for err in errs {
150-
let err_printed = format!("{err}");
150+
let err_printed = format!("{}", err.for_display());
151151
let wrapped_err: Result<(), _> = Err(err);
152152
s.push_str(&format!("{wrapped_err:#?}\n\n{err_printed}\n---\n"));
153153
}

‎crates/isograph_lang_parser/src/parse_iso_literal.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ pub fn parse_iso_literal(
4646
definition_file_path: RelativePathToSourceFile,
4747
const_export_name: Option<String>,
4848
// TODO we should not pass the text source here! Whenever the iso literal
49-
// moves around the page, we break memoization, due to this parameter.
49+
// moves around the page, we break memoizaton, due to this parameter.
5050
text_source: TextSource,
5151
) -> Result<IsoLiteralExtractionResult, WithLocation<IsographLiteralParseError>> {
5252
let mut tokens = PeekableLexer::new(&iso_literal_text);

0 commit comments

Comments
 (0)