Skip to content

Commit 8ce7b1b

Browse files
tnineslingtninesling
authored
chore(composition): Cleanup unused location parameters in mismatch reporter (#8707)
1 parent e327eed commit 8ce7b1b

File tree

10 files changed

+44
-54
lines changed

10 files changed

+44
-54
lines changed

‎apollo-federation/src/merger/compose_directive_manager.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -622,7 +622,7 @@ impl ErrorReporter {
622622
(idx, item_in_this_subgraph.cloned())
623623
})
624624
.collect();
625-
self.report_mismatch_error_without_supergraph::<MergeDirectiveItem, ()>(
625+
self.report_mismatch_error_without_supergraph::<MergeDirectiveItem>(
626626
CompositionError::DirectiveCompositionError {
627627
message: "Composed directive is not named consistently in all subgraphs"
628628
.to_string(),

‎apollo-federation/src/merger/error_reporter.rs‎

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
use std::fmt::Display;
21
use std::ops::Range;
32

43
use apollo_compiler::parser::LineColumn;
@@ -70,22 +69,22 @@ impl ErrorReporter {
7069
(self.errors, self.hints)
7170
}
7271

73-
pub(crate) fn report_mismatch_error<D: Display, S, L>(
72+
pub(crate) fn report_mismatch_error<D, S>(
7473
&mut self,
7574
error: CompositionError,
7675
mismatched_element: &D,
7776
subgraph_elements: &Sources<S>,
7877
supergraph_mismatch_accessor: impl Fn(&D) -> Option<String>,
7978
subgraph_mismatch_accessor: impl Fn(&S, usize) -> Option<String>,
8079
) {
81-
self.report_mismatch::<D, S, L>(
80+
self.report_mismatch::<D, S>(
8281
Some(mismatched_element),
8382
subgraph_elements,
8483
supergraph_mismatch_accessor,
8584
subgraph_mismatch_accessor,
8685
|elt, names| format!("{} in {}", elt, names.unwrap_or("undefined".to_string())),
8786
|elt, names| format!("{elt} in {names}"),
88-
|myself, distribution, _| {
87+
|myself, distribution| {
8988
let distribution_str = join_strings(
9089
distribution.iter(),
9190
JoinStringsOptions {
@@ -101,20 +100,20 @@ impl ErrorReporter {
101100
);
102101
}
103102

104-
pub(crate) fn report_mismatch_error_without_supergraph<T: Display, U>(
103+
pub(crate) fn report_mismatch_error_without_supergraph<T>(
105104
&mut self,
106105
error: CompositionError,
107106
subgraph_elements: &Sources<T>,
108107
mismatch_accessor: impl Fn(&T, usize) -> Option<String>,
109108
) {
110-
self.report_mismatch::<String, T, U>(
109+
self.report_mismatch::<String, T>(
111110
None,
112111
subgraph_elements,
113112
|_| None,
114113
mismatch_accessor,
115114
|_, _| String::new(),
116115
|elt, names| format!("{elt} in {names}"),
117-
|myself, distribution, _: Vec<U>| {
116+
|myself, distribution| {
118117
let distribution_str = join_strings(
119118
distribution.iter(),
120119
JoinStringsOptions {
@@ -131,7 +130,7 @@ impl ErrorReporter {
131130
}
132131

133132
#[allow(clippy::too_many_arguments)]
134-
pub(crate) fn report_mismatch_error_with_specifics<D: Display, S, L>(
133+
pub(crate) fn report_mismatch_error_with_specifics<D, S>(
135134
&mut self,
136135
error: CompositionError,
137136
mismatched_element: &D,
@@ -142,14 +141,14 @@ impl ErrorReporter {
142141
other_elements_printer: impl Fn(&str, &str) -> String,
143142
include_missing_sources: bool,
144143
) {
145-
self.report_mismatch::<D, S, L>(
144+
self.report_mismatch::<D, S>(
146145
Some(mismatched_element),
147146
subgraph_elements,
148147
supergraph_mismatch_accessor,
149148
subgraph_mismatch_accessor,
150149
supergraph_element_printer,
151150
other_elements_printer,
152-
|myself, mut distribution, _| {
151+
|myself, mut distribution| {
153152
let mut distribution_str = distribution.remove(0);
154153
distribution_str.push_str(&join_strings(
155154
distribution.iter(),
@@ -167,7 +166,7 @@ impl ErrorReporter {
167166
}
168167

169168
#[allow(clippy::too_many_arguments)]
170-
pub(crate) fn report_mismatch_hint<D: Display, S, L>(
169+
pub(crate) fn report_mismatch_hint<D, S>(
171170
&mut self,
172171
code: HintCode,
173172
message: String,
@@ -180,14 +179,14 @@ impl ErrorReporter {
180179
include_missing_sources: bool,
181180
no_end_of_message_dot: bool,
182181
) {
183-
self.report_mismatch::<D, S, L>(
182+
self.report_mismatch::<D, S>(
184183
Some(supergraph_element),
185184
subgraph_elements,
186185
supergraph_element_to_string,
187186
subgraph_element_to_string,
188187
supergraph_element_printer,
189188
other_elements_printer,
190-
|myself, mut distribution, _| {
189+
|myself, mut distribution| {
191190
let mut distribution_str = distribution.remove(0);
192191
distribution_str.push_str(&join_strings(
193192
distribution.iter(),
@@ -211,11 +210,8 @@ impl ErrorReporter {
211210

212211
/// Reports a mismatch between a supergraph element and subgraph elements.
213212
/// Not meant to be used directly: use `report_mismatch_error` or `report_mismatch_hint` instead.
214-
///
215-
/// TODO: The generic parameter `L` is meant to represent AST nodes (or locations) that are attached to error messages.
216-
/// When we decide on an implementation for those, they should be added to `ast_nodes` below.
217213
#[allow(clippy::too_many_arguments)]
218-
fn report_mismatch<D: Display, S, L>(
214+
fn report_mismatch<D, S>(
219215
&mut self,
220216
supergraph_element: Option<&D>,
221217
subgraph_elements: &Sources<S>,
@@ -227,12 +223,10 @@ impl ErrorReporter {
227223
subgraph_mismatch_accessor: impl Fn(&S, usize) -> Option<String>,
228224
supergraph_element_printer: impl Fn(&str, Option<String>) -> String,
229225
other_elements_printer: impl Fn(&str, &str) -> String,
230-
reporter: impl FnOnce(&mut Self, Vec<String>, Vec<L>),
226+
reporter: impl FnOnce(&mut Self, Vec<String>),
231227
include_missing_sources: bool,
232228
) {
233229
let mut distribution_map = Default::default();
234-
#[allow(unused_mut)] // We need this to be mutable when we decide how to handle AST nodes
235-
let mut locations: Vec<L> = Vec::new();
236230
let process_subgraph_element =
237231
|name: &str,
238232
idx: usize,
@@ -244,7 +238,6 @@ impl ErrorReporter {
244238
.or_default()
245239
.push(name.to_string());
246240
}
247-
// TODO: Get AST node equivalent and push onto `locations`
248241
};
249242
if include_missing_sources {
250243
for (i, name) in self.names.iter().enumerate() {
@@ -267,13 +260,11 @@ impl ErrorReporter {
267260
let supergraph_mismatch = supergraph_element
268261
.and_then(supergraph_mismatch_accessor)
269262
.unwrap_or_default();
270-
assert!(
271-
distribution_map.len() > 1,
272-
"Should not have been called for {}",
273-
supergraph_element
274-
.map(|elt| elt.to_string())
275-
.unwrap_or_else(|| "undefined".to_string())
276-
);
263+
if distribution_map.len() <= 1 {
264+
tracing::warn!(
265+
"report_mismatch called but no mismatch found for element {supergraph_mismatch}",
266+
);
267+
}
277268
let mut distribution = Vec::new();
278269
let subgraphs_like_supergraph = distribution_map.get(&supergraph_mismatch);
279270
// We always add the "supergraph" first (proper formatting of hints rely on this in particular)
@@ -288,6 +279,6 @@ impl ErrorReporter {
288279
let names_str = human_readable_subgraph_names(names.iter());
289280
distribution.push(other_elements_printer(v, &names_str));
290281
}
291-
reporter(self, distribution, locations);
282+
reporter(self, distribution);
292283
}
293284
}

‎apollo-federation/src/merger/merge_argument.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ impl Merger {
217217
})
218218
.collect();
219219

220-
self.error_reporter.report_mismatch_hint::<T::ArgumentPosition, &Node<InputValueDefinition>, ()>(
220+
self.error_reporter.report_mismatch_hint::<T::ArgumentPosition, &Node<InputValueDefinition>>(
221221
HintCode::InconsistentArgumentPresence,
222222
format!(
223223
"Optional argument \"{}\" will not be included in the supergraph as it does not appear in all subgraphs: ",
@@ -344,7 +344,7 @@ impl Merger {
344344
};
345345

346346
if is_incompatible {
347-
self.error_reporter.report_mismatch_error::<Node<Value>, T, ()>(
347+
self.error_reporter.report_mismatch_error::<Node<Value>, T>(
348348
if T::is_input_field() {
349349
CompositionError::InputFieldDefaultMismatch {
350350
message: format!("Input field \"{dest}\" has incompatible default values across subgraphs: it has "),
@@ -366,7 +366,7 @@ impl Merger {
366366
},
367367
);
368368
} else if is_inconsistent {
369-
self.error_reporter.report_mismatch_hint::<Node<Value>, T, ()>(
369+
self.error_reporter.report_mismatch_hint::<Node<Value>, T>(
370370
HintCode::InconsistentDefaultValuePresence,
371371
format!("Argument \"{dest}\" has a default value in only some subgraphs: "),
372372
dest_default,

‎apollo-federation/src/merger/merge_directive.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ impl Merger {
226226
))
227227
}
228228
}
229-
self.error_reporter.report_mismatch_hint::<Directive, DirectiveTargetPosition, ()>(
229+
self.error_reporter.report_mismatch_hint::<Directive, DirectiveTargetPosition>(
230230
HintCode::InconsistentNonRepeatableDirectiveArguments,
231231
format!("Non-repeatable directive @{name} is applied to \"{dest}\" in multiple subgraphs but with incompatible arguments. "),
232232
&most_used_directive,
@@ -402,7 +402,7 @@ impl Merger {
402402
// An executable directive could appear in any place of a query and thus get to any subgraph, so we cannot keep an
403403
// executable directive unless it is in all subgraphs. We use an 'intersection' strategy.
404404
dest.remove(&mut self.merged)?;
405-
self.error_reporter.report_mismatch_hint::<DirectiveDefinitionPosition, DirectiveDefinitionPosition,()>(
405+
self.error_reporter.report_mismatch_hint::<DirectiveDefinitionPosition, DirectiveDefinitionPosition>(
406406
HintCode::InconsistentExecutableDirectivePresence,
407407
format!("Executable directive \"@{name}\" will not be part of the supergraph as it does not appear in all subgraphs: "),
408408
dest,
@@ -444,7 +444,7 @@ impl Merger {
444444
self.subgraphs[*idx].name, locations
445445
);
446446
if locations.is_empty() {
447-
self.error_reporter.report_mismatch_hint::<DirectiveDefinitionPosition, DirectiveDefinitionPosition, ()>(
447+
self.error_reporter.report_mismatch_hint::<DirectiveDefinitionPosition, DirectiveDefinitionPosition>(
448448
HintCode::NoExecutableDirectiveLocationsIntersection,
449449
format!("Executable directive \"@{name}\" has no location that is common to all subgraphs: "),
450450
dest,
@@ -467,7 +467,7 @@ impl Merger {
467467
let supergraph_dest = dest.get(self.merged.schema())?;
468468

469469
if inconsistent_repeatable {
470-
self.error_reporter.report_mismatch_hint::<Node<DirectiveDefinition>, DirectiveDefinitionPosition, ()>(
470+
self.error_reporter.report_mismatch_hint::<Node<DirectiveDefinition>, DirectiveDefinitionPosition>(
471471
HintCode::InconsistentExecutableDirectiveRepeatable,
472472
format!("Executable directive \"@{name}\" will not be marked repeatable in the supergraph as it is inconsistently marked repeatable in subgraphs: "),
473473
supergraph_dest,
@@ -482,7 +482,7 @@ impl Merger {
482482
);
483483
}
484484
if inconsistent_locations {
485-
self.error_reporter.report_mismatch_hint::<Node<DirectiveDefinition>, DirectiveDefinitionPosition, ()>(
485+
self.error_reporter.report_mismatch_hint::<Node<DirectiveDefinition>, DirectiveDefinitionPosition>(
486486
HintCode::InconsistentExecutableDirectiveLocations,
487487
format!(
488488
"Executable directive \"@{name}\" has inconsistent locations across subgraphs "

‎apollo-federation/src/merger/merge_enum.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ impl Merger {
169169
input_example,
170170
output_example,
171171
} if violates_intersection_requirement => {
172-
self.error_reporter.report_mismatch_error_with_specifics::<_, _, ()>(
172+
self.error_reporter.report_mismatch_error_with_specifics::<_, _>(
173173
CompositionError::EnumValueMismatch {
174174
message: format!(
175175
"Enum type \"{}\" is used as both input type (for example, as type of \"{}\") and output type (for example, as type of \"{}\"), but value \"{}\" is not defined in all the subgraphs defining \"{}\": ",
@@ -193,7 +193,7 @@ impl Merger {
193193
);
194194
}
195195
EnumTypeUsage::Input { .. } if violates_intersection_requirement => {
196-
self.error_reporter.report_mismatch_hint::<_, _, ()>(
196+
self.error_reporter.report_mismatch_hint::<_, _>(
197197
HintCode::InconsistentEnumValueForInputEnum,
198198
format!(
199199
"Value \"{}\" of enum type \"{}\" will not be part of the supergraph as it is not defined in all the subgraphs defining \"{}\": ", value_pos.value_name, value_pos.type_name, value_pos.type_name
@@ -264,7 +264,7 @@ impl Merger {
264264
// As soon as we find a subgraph that has the type but not the member, we hint.
265265
for enum_type in sources.values().flatten() {
266266
if !enum_type.values.contains_key(value_name) {
267-
self.error_reporter.report_mismatch_hint::<_, _, ()>(
267+
self.error_reporter.report_mismatch_hint::<_, _>(
268268
HintCode::InconsistentEnumValueForOutputEnum,
269269
format!(
270270
"Value \"{value_name}\" of enum type \"{dest_name}\" has been added to the supergraph but is only defined in a subset of the subgraphs defining \"{dest_name}\": ",

‎apollo-federation/src/merger/merge_field.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -530,7 +530,7 @@ impl Merger {
530530

531531
// Phase 2: Reporting - report errors in groups, matching JS version order
532532
if has_invalid_types {
533-
self.error_reporter.report_mismatch_error::<FieldDefinition, FieldDefinitionPosition, ()>(
533+
self.error_reporter.report_mismatch_error::<FieldDefinition, FieldDefinitionPosition>(
534534
CompositionError::ExternalTypeMismatch {
535535
message: format!(
536536
"Type of field \"{dest}\" is incompatible across subgraphs (where marked @external): it has ",
@@ -562,7 +562,7 @@ impl Merger {
562562
field_name: dest.field_name().clone(),
563563
argument_name: arg_name.clone(),
564564
};
565-
self.error_reporter.report_mismatch_error::<ObjectFieldArgumentDefinitionPosition, ObjectFieldArgumentDefinitionPosition, ()>(
565+
self.error_reporter.report_mismatch_error::<ObjectFieldArgumentDefinitionPosition, ObjectFieldArgumentDefinitionPosition>(
566566
CompositionError::ExternalArgumentTypeMismatch {
567567
message: format!(
568568
"Type of argument \"{argument_pos}\" is incompatible across subgraphs (where \"{dest}\" is marked @external): it has ",
@@ -581,7 +581,7 @@ impl Merger {
581581
field_name: dest.field_name().clone(),
582582
argument_name: arg_name.clone(),
583583
};
584-
self.error_reporter.report_mismatch_error::<ObjectFieldArgumentDefinitionPosition, ObjectFieldArgumentDefinitionPosition, ()>(
584+
self.error_reporter.report_mismatch_error::<ObjectFieldArgumentDefinitionPosition, ObjectFieldArgumentDefinitionPosition>(
585585
CompositionError::ExternalArgumentDefaultMismatch {
586586
message: format!(
587587
"Argument \"{argument_pos}\" has incompatible defaults across subgraphs (where \"{dest}\" is marked @external): it has ",

‎apollo-federation/src/merger/merge_input.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ impl Merger {
122122
}
123123
}
124124

125-
self.error_reporter.report_mismatch_hint::<InputObjectFieldDefinitionPosition, InputObjectFieldDefinitionPosition, ()>(
125+
self.error_reporter.report_mismatch_hint::<InputObjectFieldDefinitionPosition, InputObjectFieldDefinitionPosition>(
126126
HintCode::InconsistentInputObjectField,
127127
format!("Input object field \"{}\" will not be added to \"{}\" in the supergraph as it does not appear in all subgraphs: ",
128128
dest_field.field_name, dest.type_name

‎apollo-federation/src/merger/merge_links.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ impl Merger {
177177
)
178178
})
179179
.collect();
180-
self.error_reporter.report_mismatch_error::<_, _, ()>(
180+
self.error_reporter.report_mismatch_error::<_, _>(
181181
CompositionError::LinkImportNameMismatch {
182182
message: format!("The \"@{}\" directive (from {}) is imported with mismatched name between subgraphs: it is imported as ", directive.name, subgraph_core_directive.url),
183183
},

‎apollo-federation/src/merger/merge_union.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ impl Merger {
8585
for union_type in sources.values().flatten() {
8686
// As soon as we find a subgraph that has the union type but not the member, we hint
8787
if !union_type.members.contains(member_name) {
88-
self.error_reporter.report_mismatch_hint::<UnionTypeDefinitionPosition, Node<UnionType>, ()>(
88+
self.error_reporter.report_mismatch_hint::<UnionTypeDefinitionPosition, Node<UnionType>>(
8989
HintCode::InconsistentUnionMember,
9090
format!(
9191
"Union type \"{}\" includes member type \"{}\" in some but not all defining subgraphs: ",

0 commit comments

Comments
 (0)