Skip to content

Commit

Permalink
Add a warning when a component/type name overwrite another
Browse files Browse the repository at this point in the history
Also fix the unused component warning when that happens
Fixes #7176

ChangeLog: Warning when a type name overwrite another
  • Loading branch information
ogoffart committed Jan 3, 2025
1 parent 3a4864c commit 12c99f1
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 17 deletions.
35 changes: 28 additions & 7 deletions internal/compiler/object_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ impl Document {
diag: &mut BuildDiagnostics,
local_registry: &mut TypeRegister| {
let compo = Component::from_node(n, diag, local_registry);
local_registry.add(compo.clone());
if !local_registry.add(compo.clone()) {
diag.push_warning(format!("Component '{}' is replacing a previously defined component with the same name", compo.id), &syntax_nodes::Component::from(compo.node.clone().unwrap()).DeclaredIdentifier());
}
inner_components.push(compo);
};
let process_struct = |n: syntax_nodes::StructDeclaration,
Expand All @@ -102,7 +104,14 @@ impl Document {
parser::identifier_text(&n.DeclaredIdentifier()),
);
assert!(matches!(ty, Type::Struct(_)));
local_registry.insert_type(ty.clone());
if !local_registry.insert_type(ty.clone()) {
diag.push_warning(
format!(
"Struct '{ty}' is replacing a previously defined type with the same name"
),
&n.DeclaredIdentifier(),
);
}
inner_types.push(ty);
};
let process_enum = |n: syntax_nodes::EnumDeclaration,
Expand Down Expand Up @@ -132,9 +141,17 @@ impl Document {
}
})
.collect();
let en = Enumeration { name: name.clone(), values, default_value: 0, node: Some(n) };
let en =
Enumeration { name: name.clone(), values, default_value: 0, node: Some(n.clone()) };
let ty = Type::Enumeration(Rc::new(en));
local_registry.insert_type_with_name(ty.clone(), name);
if !local_registry.insert_type_with_name(ty.clone(), name.clone()) {
diag.push_warning(
format!(
"Enum '{name}' is replacing a previously defined type with the same name"
),
&n.DeclaredIdentifier(),
);
}
inner_types.push(ty);
};

Expand Down Expand Up @@ -226,9 +243,7 @@ impl Document {
if local_compo.is_global() {
continue;
}
// First ref count is in the type registry, the second one in inner_components. Any use of the element
// would have resulted in another strong reference.
if Rc::strong_count(local_compo) == 2 {
if !local_compo.used.get() {
diag.push_warning(
"Component is neither used nor exported".into(),
&local_compo.node,
Expand Down Expand Up @@ -377,6 +392,9 @@ pub struct Component {
/// if it is a global singleton and exported.
pub exported_global_names: RefCell<Vec<ExportedName>>,

/// True if this component is used as a sub-component by at least one other component.
pub used: Cell<bool>,

/// The list of properties (name and type) declared as private in the component.
/// This is used to issue better error in the generated code if the property is used.
pub private_properties: RefCell<Vec<(SmolStr, Type)>>,
Expand Down Expand Up @@ -962,6 +980,9 @@ impl Element {
};
// This isn't truly qualified yet, the enclosing component is added at the end of Component::from_node
let qualified_id = (!id.is_empty()).then(|| id.clone());
if let ElementType::Component(c) = &base_type {
c.used.set(true);
}
let type_name = base_type
.type_name()
.filter(|_| base_type != tr.empty_type())
Expand Down
1 change: 1 addition & 0 deletions internal/compiler/passes/inlining.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ fn duplicate_sub_component(
popup_windows: Default::default(),
timers: component_to_duplicate.timers.clone(),
exported_global_names: component_to_duplicate.exported_global_names.clone(),
used: component_to_duplicate.used.clone(),
private_properties: Default::default(),
inherits_popup_window: core::cell::Cell::new(false),
};
Expand Down
47 changes: 47 additions & 0 deletions internal/compiler/tests/syntax/lookup/name_reuse.slint
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright © SixtyFPS GmbH <info@slint.dev>
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0

component Foo /* Foo 1 */ { }

export component FooBaz {
Foo /* <- TEST_ME_1 */ { }
}

component Foo /* Foo 2 */ { }
// ^warning{Component 'Foo' is replacing a previously defined component with the same name}

export component Baz {
Foo /* <- TEST_ME_2 */ { }
}


struct Type1 { xxx: int }
enum Type2 { AAA, BBB }

component Test1 {}
//^warning{Component is neither used nor exported}

export component Test1 {
// ^warning{Component 'Test1' is replacing a previously defined component with the same name}
property <Type1> t1: { xxx: 42 };
property <Type2> t2: Type2.AAA;
// ^error{Cannot access id 'Type2'} // because in the lookup phase it was already erased

init => {
debug(Type1.CCC); // This is allowed because the resolving phase has full view on the document
debug(Type2.AAA);
// ^error{Cannot access id 'Type2'}
}
}

struct Type2 { yyy: int }
// ^warning{Struct 'Type2' is replacing a previously defined type with the same name}
enum Type1 { CCC, DDD }
// ^warning{Enum 'Type1' is replacing a previously defined type with the same name}
export component Test2 {
property <Type1> t1: Type1.DDD;
property <Type2> t2: { yyy: 42 };
property <Type1> error: Type2.AAA;
// ^error{Cannot access id 'Type2'}
}

3 changes: 2 additions & 1 deletion internal/compiler/typeloader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ impl Snapshotter {
exported_global_names: RefCell::new(
component.exported_global_names.borrow().clone(),
),
used: component.used.clone(),
init_code: RefCell::new(component.init_code.borrow().clone()),
inherits_popup_window: std::cell::Cell::new(component.inherits_popup_window.get()),
optimized_elements,
Expand Down Expand Up @@ -1455,7 +1456,7 @@ impl TypeLoader {
itertools::Either::Right(ty) => registry_to_populate
.borrow_mut()
.insert_type_with_name(ty, import_name.internal_name),
}
};
}
}

Expand Down
29 changes: 20 additions & 9 deletions internal/compiler/typeregister.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,12 +356,17 @@ impl TypeRegister {
}
}

/// FIXME: same as 'add' ?
pub fn insert_type(&mut self, t: Type) {
self.types.insert(t.to_smolstr(), t);
/// Insert a type into the type register with its builtin type name.
///
/// Returns false if a it replaced an existing type.
pub fn insert_type(&mut self, t: Type) -> bool {
self.types.insert(t.to_smolstr(), t).is_none()
}
pub fn insert_type_with_name(&mut self, t: Type, name: SmolStr) {
self.types.insert(name, t);
/// Insert a type into the type register with a specified name.
///
/// Returns false if a it replaced an existing type.
pub fn insert_type_with_name(&mut self, t: Type, name: SmolStr) -> bool {
self.types.insert(name, t).is_none()
}

fn builtin_internal() -> Self {
Expand Down Expand Up @@ -642,12 +647,18 @@ impl TypeRegister {
self.lookup(qualified[0].as_ref())
}

pub fn add(&mut self, comp: Rc<Component>) {
self.add_with_name(comp.id.clone(), comp);
/// Add the component with it's defined name
///
/// Returns false if there was already an element with the same name
pub fn add(&mut self, comp: Rc<Component>) -> bool {
self.add_with_name(comp.id.clone(), comp)
}

pub fn add_with_name(&mut self, name: SmolStr, comp: Rc<Component>) {
self.elements.insert(name, ElementType::Component(comp));
/// Add the component with a specified name
///
/// Returns false if there was already an element with the same name
pub fn add_with_name(&mut self, name: SmolStr, comp: Rc<Component>) -> bool {
self.elements.insert(name, ElementType::Component(comp)).is_none()
}

pub fn add_builtin(&mut self, builtin: Rc<BuiltinElement>) {
Expand Down

0 comments on commit 12c99f1

Please sign in to comment.