Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API review of the slint interpreter Compiler api #5556

Merged
merged 1 commit into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/node/src/interpreter/component_compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ impl JsComponentCompiler {
pub fn build_from_path(&mut self, path: String) -> HashMap<String, JsComponentDefinition> {
let r = spin_on::spin_on(self.internal.build_from_path(PathBuf::from(path)));
self.diagnostics = r.diagnostics().collect();
r.component_names().filter_map(|n| Some((n.to_owned(), r.component(n)?.into()))).collect()
r.components().map(|c| (c.name().to_owned(), c.into())).collect()
}

/// Compile some .slint code into a ComponentDefinition
Expand All @@ -118,6 +118,6 @@ impl JsComponentCompiler {
) -> HashMap<String, JsComponentDefinition> {
let r = spin_on::spin_on(self.internal.build_from_source(source_code, PathBuf::from(path)));
self.diagnostics = r.diagnostics().collect();
r.component_names().filter_map(|n| Some((n.to_owned(), r.component(n)?.into()))).collect()
r.components().map(|c| (c.name().to_owned(), c.into())).collect()
}
}
4 changes: 2 additions & 2 deletions api/rs/build/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ pub fn compile_with_config(
let mut diag = BuildDiagnostics::default();
let syntax_node = i_slint_compiler::parser::parse_file(&path, &mut diag);

if diag.has_error() {
if diag.has_errors() {
let vec = diag.to_string_vec();
diag.print();
return Err(CompileError::CompileError(vec));
Expand All @@ -377,7 +377,7 @@ pub fn compile_with_config(
let (doc, diag, loader) =
spin_on::spin_on(i_slint_compiler::compile_syntax_node(syntax_node, diag, compiler_config));

if diag.has_error() {
if diag.has_errors() {
let vec = diag.to_string_vec();
diag.print();
return Err(CompileError::CompileError(vec));
Expand Down
4 changes: 2 additions & 2 deletions api/rs/macros/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ pub fn slint(stream: TokenStream) -> TokenStream {
};
let mut diag = BuildDiagnostics::default();
let syntax_node = parser::parse_tokens(tokens.clone(), source_file, &mut diag);
if diag.has_error() {
if diag.has_errors() {
return diag.report_macro_diagnostic(&tokens);
}

Expand All @@ -397,7 +397,7 @@ pub fn slint(stream: TokenStream) -> TokenStream {
let (root_component, diag, loader) =
spin_on::spin_on(compile_syntax_node(syntax_node, diag, compiler_config));
//println!("{:#?}", tree);
if diag.has_error() {
if diag.has_errors() {
return diag.report_macro_diagnostic(&tokens);
}

Expand Down
8 changes: 4 additions & 4 deletions internal/compiler/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ impl BuildDiagnostics {
}

/// Return true if there is at least one compilation error for this file
pub fn has_error(&self) -> bool {
pub fn has_errors(&self) -> bool {
self.inner.iter().any(|diag| diag.level == DiagnosticLevel::Error)
}

Expand Down Expand Up @@ -467,7 +467,7 @@ impl BuildDiagnostics {
span_map: &[crate::parser::Token],
) -> proc_macro::TokenStream {
let mut result = proc_macro::TokenStream::default();
let mut needs_error = self.has_error();
let mut needs_error = self.has_errors();
self.call_diagnostics(
&mut (),
Some(&mut |diag| {
Expand Down Expand Up @@ -543,7 +543,7 @@ impl BuildDiagnostics {
#[cfg(feature = "display-diagnostics")]
#[must_use]
pub fn check_and_exit_on_error(self) -> Self {
if self.has_error() {
if self.has_errors() {
self.print();
std::process::exit(-1);
}
Expand All @@ -552,7 +552,7 @@ impl BuildDiagnostics {

#[cfg(feature = "display-diagnostics")]
pub fn print_warnings_and_exit_on_error(self) {
let has_error = self.has_error();
let has_error = self.has_errors();
self.print();
if has_error {
std::process::exit(-1);
Expand Down
24 changes: 14 additions & 10 deletions internal/compiler/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,24 @@ pub enum EmbedResourcesKind {
EmbedTextures,
}

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum ComponentsToGenerate {
/// All exported Windows.
#[derive(Clone, Debug, Eq, PartialEq, Default)]
#[non_exhaustive]
pub enum ComponentSelection {
/// All components that inherit from Window.
///
/// When set, there will be a warning if an exported component is not a window.
AllExportedWindows,
/// Note: Components marked for export but lacking Window inheritance are not selected (this will produce a warning),
/// For compatibility reason, the last exported component is still selected even if it doesn't inherit Window,
/// and if no component is exported, the last component is selected
#[default]
ExportedWindows,
/// The Last component (legacy for the viewer / interpreter)

/// Only the last exported component is generated, regardless if this is a Window or not,
/// (and it will be transformed in a Window)
LastComponent,
LastExported,

/// The component with the given name is generated
ComponentWithName(String),
Named(String),
}

/// CompilationConfiguration allows configuring different aspects of the compiler.
Expand Down Expand Up @@ -120,7 +124,7 @@ pub struct CompilerConfiguration {
/// Generate debug information for elements (ids, type names)
pub debug_info: bool,

pub components_to_generate: ComponentsToGenerate,
pub components_to_generate: ComponentSelection,
}

impl CompilerConfiguration {
Expand Down Expand Up @@ -195,7 +199,7 @@ impl CompilerConfiguration {
translation_domain: None,
cpp_namespace,
debug_info,
components_to_generate: ComponentsToGenerate::AllExportedWindows,
components_to_generate: ComponentSelection::ExportedWindows,
}
}
}
Expand Down Expand Up @@ -244,7 +248,7 @@ pub async fn compile_syntax_node(
&type_registry,
);

if !diagnostics.has_error() {
if !diagnostics.has_errors() {
passes::run_passes(&mut doc, &mut loader, false, &mut diagnostics).await;
} else {
// Don't run all the passes in case of errors because because some invariants are not met.
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/load_builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ fn compiled(
&mut crate::lookup::LookupCtx::empty_context(type_register, &mut diag),
)
.maybe_convert_to(ty, &node, &mut diag);
if diag.has_error() {
if diag.has_errors() {
let vec = diag.to_string_vec();
#[cfg(feature = "display-diagnostics")]
diag.print();
Expand Down
16 changes: 8 additions & 8 deletions internal/compiler/object_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ macro_rules! unwrap_or_continue {
match $e {
Some(x) => x,
None => {
debug_assert!($diag.has_error()); // error should have been reported at parsing time
debug_assert!($diag.has_errors()); // error should have been reported at parsing time
continue;
}
}
Expand Down Expand Up @@ -91,7 +91,7 @@ impl Document {
if let Type::Struct { name, .. } = &mut ty {
*name = parser::identifier_text(&n.DeclaredIdentifier());
} else {
assert!(diag.has_error());
assert!(diag.has_errors());
return;
}
local_registry.insert_type(ty.clone());
Expand All @@ -102,7 +102,7 @@ impl Document {
local_registry: &mut TypeRegister,
inner_types: &mut Vec<Type>| {
let Some(name) = parser::identifier_text(&n.DeclaredIdentifier()) else {
assert!(diag.has_error());
assert!(diag.has_errors());
return;
};
let mut existing_names = HashSet::new();
Expand Down Expand Up @@ -929,7 +929,7 @@ impl Element {
ElementType::Global
} else if parent_type != ElementType::Error {
// This should normally never happen because the parser does not allow for this
assert!(diag.has_error());
assert!(diag.has_errors());
return ElementRc::default();
} else {
tr.empty_type()
Expand Down Expand Up @@ -1195,7 +1195,7 @@ impl Element {
.insert(name.clone(), BindingExpression::new_uncompiled(func.clone().into()).into())
.is_some()
{
assert!(diag.has_error());
assert!(diag.has_errors());
}

let mut visibility = PropertyVisibility::Private;
Expand Down Expand Up @@ -1868,7 +1868,7 @@ pub fn type_from_node(
} else if let Some(array_node) = node.ArrayType() {
Type::Array(Box::new(type_from_node(array_node.Type(), diag, tr)))
} else {
assert!(diag.has_error());
assert!(diag.has_errors());
Type::Invalid
}
}
Expand Down Expand Up @@ -2490,7 +2490,7 @@ impl Exports {
let name_ident: SyntaxNode = component.DeclaredIdentifier().into();
let name =
parser::identifier_text(&component.DeclaredIdentifier()).unwrap_or_else(|| {
debug_assert!(diag.has_error());
debug_assert!(diag.has_errors());
String::new()
});

Expand All @@ -2512,7 +2512,7 @@ impl Exports {
})
.filter_map(|name_ident| {
let name = parser::identifier_text(&name_ident).unwrap_or_else(|| {
debug_assert!(diag.has_error());
debug_assert!(diag.has_errors());
String::new()
});

Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/parser-test-macro/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ fn generate_test(fn_name: &str, doc: &str, extra_args: usize) -> String {
let mut diag = Default::default();
let mut p = DefaultParser::new("{line}", &mut diag);
{fn_name}(&mut p{follow_args});
let has_error = p.diags.has_error();
let has_error = p.diags.has_errors();
//#[cfg(feature = "display-diagnostics")]
//p.diags.print();
assert!(!has_error);
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,7 @@ pub fn parse_expression_as_bindingexpression(
};
let node = rowan::SyntaxNode::new_root(p.builder.finish());

if !build_diagnostics.has_error() && token.kind() != SyntaxKind::Eof {
if !build_diagnostics.has_errors() && token.kind() != SyntaxKind::Eof {
build_diagnostics.push_error_with_span(
format!("Expected end of string, found \"{}\"", &token.kind()),
crate::diagnostics::SourceLocation {
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ pub async fn run_passes(
remove_return::remove_return(doc);

doc.visit_all_used_components(|component| {
if !diag.has_error() {
if !diag.has_errors() {
// binding loop causes panics in const_propagation
const_propagation::const_propagation(component);
}
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/passes/check_expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn check_expression(component: &Rc<Component>, e: &Expression, diag: &mut BuildD
match e {
Expression::MemberFunction { .. } => {
// Must already have been be reported.
debug_assert!(diag.has_error());
debug_assert!(diag.has_errors());
}
Expression::BuiltinMacroReference(_, node) => {
diag.push_error("Builtin function must be called".into(), node);
Expand Down
12 changes: 5 additions & 7 deletions internal/compiler/passes/check_public_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::rc::Rc;
use crate::diagnostics::{BuildDiagnostics, DiagnosticLevel};
use crate::langtype::ElementType;
use crate::object_tree::{Component, Document, ExportedName, PropertyVisibility};
use crate::{CompilerConfiguration, ComponentsToGenerate};
use crate::{CompilerConfiguration, ComponentSelection};
use itertools::Either;

pub fn check_public_api(
Expand All @@ -18,9 +18,7 @@ pub fn check_public_api(
) {
let last = doc.last_exported_component();

if last.is_none()
&& !matches!(&config.components_to_generate, ComponentsToGenerate::ComponentWithName(_))
{
if last.is_none() && !matches!(&config.components_to_generate, ComponentSelection::Named(_)) {
let last_imported = doc
.node
.as_ref()
Expand All @@ -36,7 +34,7 @@ pub fn check_public_api(
}

match &config.components_to_generate {
ComponentsToGenerate::AllExportedWindows => doc.exports.retain(|export| {
ComponentSelection::ExportedWindows => doc.exports.retain(|export| {
// Warn about exported non-window (and remove them from the export unless it's the last for compatibility)
if let Either::Left(c) = &export.1 {
if !c.is_global() && !super::ensure_window::inherits_window(c) {
Expand All @@ -52,15 +50,15 @@ pub fn check_public_api(
true
}),
// Only keep the last component if there is one
ComponentsToGenerate::LastComponent => doc.exports.retain(|export| {
ComponentSelection::LastExported => doc.exports.retain(|export| {
if let Either::Left(c) = &export.1 {
c.is_global() || last.as_ref().map_or(true, |last| Rc::ptr_eq(last, c))
} else {
true
}
}),
// Only keep the component with the given name
ComponentsToGenerate::ComponentWithName(name) => {
ComponentSelection::Named(name) => {
doc.exports.retain(|export| {
if let Either::Left(c) = &export.1 {
c.is_global() || &c.id == name
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/passes/const_propagation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ export component Foo {
);
let (doc, diag, _) =
spin_on::spin_on(crate::compile_syntax_node(doc_node, test_diags, compiler_config));
assert!(!diag.has_error());
assert!(!diag.has_errors());

let out_binding = doc
.inner_components
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/passes/default_geometry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ fn test_no_property_for_100pc() {
);
let (doc, diag, _) =
spin_on::spin_on(crate::compile_syntax_node(doc_node, test_diags, compiler_config));
assert!(!diag.has_error(), "{:?}", diag.to_string_vec());
assert!(!diag.has_errors(), "{:?}", diag.to_string_vec());

let root_elem = doc.inner_components.last().unwrap().root_element.borrow();

Expand Down
4 changes: 2 additions & 2 deletions internal/compiler/passes/focus_handling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl<'a> LocalFocusForwards<'a> {
let Expression::ElementReference(focus_target) = &forward_focus_binding.expression
else {
// resolve expressions pass has produced type errors
debug_assert!(diag.has_error());
debug_assert!(diag.has_errors());
return;
};

Expand Down Expand Up @@ -178,7 +178,7 @@ impl<'a> LocalFocusForwards<'a> {
}
if arguments.len() != 1 {
assert!(
self.diag.has_error(),
self.diag.has_errors(),
"Invalid argument generated for {} call",
focus_function.name()
);
Expand Down
8 changes: 4 additions & 4 deletions internal/compiler/passes/infer_aliases_types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,22 +71,22 @@ fn resolve_alias(
Type::InferredCallback | Type::InferredProperty
));
// It is still unresolved because there is an error in that component
assert!(diag.has_error());
assert!(diag.has_errors());
return;
}
};
drop(borrow_mut);

let borrow = elem.borrow();
let Some(binding) = borrow.bindings.get(prop) else {
assert!(diag.has_error());
assert!(diag.has_errors());
return;
};
let nr = match &binding.borrow().expression {
Expression::Uncompiled(node) => {
let Some(node) = syntax_nodes::TwoWayBinding::new(node.clone()) else {
assert!(
diag.has_error(),
diag.has_errors(),
"The parser only avoid missing types for two way bindings"
);
return;
Expand Down Expand Up @@ -135,7 +135,7 @@ fn resolve_alias(
} else if old_type == Type::InferredCallback {
if !matches!(ty, Type::Callback { .. }) {
if nr.is_some() && ty == Type::Invalid {
debug_assert!(diag.has_error());
debug_assert!(diag.has_errors());
} else {
diag.push_error(
format!("Binding to callback '{}' must bind to another callback", prop),
Expand Down
2 changes: 1 addition & 1 deletion internal/compiler/passes/lower_layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ fn lower_element_layout(
// We shouldn't lower layout if we have a Row in there. Unless the Row is the root of a repeated item,
// in which case another error has been reported
assert!(
diag.has_error()
diag.has_errors()
&& Rc::ptr_eq(&component.root_element, elem)
&& component
.parent_element
Expand Down
Loading
Loading