From decbe0ade542b1dba96098bea6a0ae66b73bc405 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Mon, 2 Nov 2020 17:16:51 +0100 Subject: [PATCH] Fix panic when exporting invalid types And also still allow to export builtin component --- sixtyfps_compiler/object_tree.rs | 90 ++++++++++--------- sixtyfps_compiler/parser/document.rs | 5 +- .../tests/syntax/imports/invalid_export.60 | 22 +++++ sixtyfps_compiler/typeloader.rs | 6 ++ 4 files changed, 78 insertions(+), 45 deletions(-) create mode 100644 sixtyfps_compiler/tests/syntax/imports/invalid_export.60 diff --git a/sixtyfps_compiler/object_tree.rs b/sixtyfps_compiler/object_tree.rs index 04dbe5129..8ced60b07 100644 --- a/sixtyfps_compiler/object_tree.rs +++ b/sixtyfps_compiler/object_tree.rs @@ -88,7 +88,7 @@ impl Document { _ => {} }; } - let exports = Exports::from_node(&node, &inner_components, &parent_registry); + let exports = Exports::from_node(&node, &inner_components, &local_registry, diag); Document { // FIXME: one should use the `component` hint instead of always returning the last @@ -1023,12 +1023,6 @@ pub struct Transition { pub property_animations: Vec<(NamedReference, ElementRc)>, } -#[derive(Debug, Clone)] -pub struct NamedExport { - pub internal_name: String, - pub exported_name: String, -} - #[derive(Default, Debug, derive_more::Deref)] pub struct Exports(Vec<(String, Rc)>); @@ -1036,8 +1030,16 @@ impl Exports { pub fn from_node( doc: &syntax_nodes::Document, inner_components: &Vec>, - type_registry: &Rc>, + type_registry: &TypeRegister, + diag: &mut FileDiagnostics, ) -> Self { + #[derive(Debug, Clone)] + struct NamedExport { + internal_name_ident: SyntaxNodeWithSourceFile, + internal_name: String, + exported_name: String, + } + let mut exports = doc .ExportsList() .flat_map(|exports| exports.ExportSpecifier()) @@ -1049,7 +1051,11 @@ impl Exports { .expect("internal error: missing external name for export"), None => internal_name.clone(), }; - Some(NamedExport { internal_name, exported_name }) + Some(NamedExport { + internal_name_ident: export_specifier.ExportIdentifier().into(), + internal_name, + exported_name, + }) }) .collect::>(); @@ -1057,57 +1063,53 @@ impl Exports { |component| { let name = identifier_text(&component.DeclaredIdentifier()) .expect("internal error: cannot export component without name"); - Some(NamedExport { internal_name: name.clone(), exported_name: name }) + Some(NamedExport { + internal_name_ident: component.DeclaredIdentifier().into(), + internal_name: name.clone(), + exported_name: name, + }) }, )); if exports.is_empty() { let internal_name = inner_components.last().cloned().unwrap_or_default().id.clone(); exports.push(NamedExport { + internal_name_ident: doc.clone().into(), internal_name: internal_name.clone(), exported_name: internal_name, }) } - let imported_names = doc - .ImportSpecifier() - .map(|import| crate::typeloader::ImportedName::extract_imported_names(&import)) - .flatten() - .collect::>(); - - let resolve_export_to_inner_component_or_import = |export: &NamedExport| { - if let Some(local_comp) = inner_components.iter().find(|c| c.id == export.internal_name) - { - local_comp.clone() - } else { - imported_names - .iter() - .find_map(|import| { - if import.internal_name == export.internal_name { - Some( - type_registry - .borrow() - .lookup_element(&import.internal_name) - .unwrap() - .as_component() - .clone(), - ) - } else { - None - } - }) - .unwrap() - } - }; + let mut resolve_export_to_inner_component_or_import = + |export: &NamedExport| match type_registry.lookup(export.internal_name.as_str()) { + Type::Component(c) => Some(c), + Type::Invalid => { + diag.push_error( + format!("'{}' not found", export.internal_name), + &export.internal_name_ident, + ); + None + } + _ => { + diag.push_error( + format!( + "Cannot export '{}' because it is not a component", + export.internal_name, + ), + &export.internal_name_ident, + ); + None + } + }; Self( exports .iter() - .map(|export| { - ( + .filter_map(|export| { + Some(( export.exported_name.clone(), - resolve_export_to_inner_component_or_import(export), - ) + resolve_export_to_inner_component_or_import(export)?, + )) }) .collect(), ) diff --git a/sixtyfps_compiler/parser/document.rs b/sixtyfps_compiler/parser/document.rs index 89aec6c98..daa67b63f 100644 --- a/sixtyfps_compiler/parser/document.rs +++ b/sixtyfps_compiler/parser/document.rs @@ -597,7 +597,10 @@ fn parse_export(p: &mut impl Parser) -> bool { p.consume(); return true; } - SyntaxKind::Eof => return false, + SyntaxKind::Eof => { + p.error("Expected comma"); + return false; + } SyntaxKind::Comma => { p.consume(); } diff --git a/sixtyfps_compiler/tests/syntax/imports/invalid_export.60 b/sixtyfps_compiler/tests/syntax/imports/invalid_export.60 new file mode 100644 index 000000000..ccb9a74d3 --- /dev/null +++ b/sixtyfps_compiler/tests/syntax/imports/invalid_export.60 @@ -0,0 +1,22 @@ + /* LICENSE BEGIN + This file is part of the SixtyFPS Project -- https://sixtyfps.io + Copyright (c) 2020 Olivier Goffart + Copyright (c) 2020 Simon Hausmann + + SPDX-License-Identifier: GPL-3.0-only + This file is also available under commercial licensing terms. + Please contact info@sixtyfps.io for more information. +LICENSE END */ + +export { Foo as Bar } +// ^error{'Foo' not found} + +export { Image as Plop } +// ^error{Cannot export 'Image' because it is not a component} + +export { string as Boob } +// ^error{Cannot export 'string' because it is not a component} + +Hello := Plop { +// ^error{Unknown type Plop} +} \ No newline at end of file diff --git a/sixtyfps_compiler/typeloader.rs b/sixtyfps_compiler/typeloader.rs index 2d4c2bfc6..6b4559951 100644 --- a/sixtyfps_compiler/typeloader.rs +++ b/sixtyfps_compiler/typeloader.rs @@ -75,6 +75,7 @@ struct ImportedTypes { type DependenciesByFile = BTreeMap; +#[derive(Debug)] pub struct ImportedName { // name of export to match in the other file pub external_name: String, @@ -161,6 +162,11 @@ impl<'a> TypeLoader<'a> { dependency_diagnostics.current_path = SourceFile::new(path.clone()); + if dependency_diagnostics.has_error() { + self.build_diagnostics.add(dependency_diagnostics); + return; + } + let dependency_doc: syntax_nodes::Document = dependency_doc.into(); let dependency_registry =