Add comprehensive linting #1327
+468
−485
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,8 +65,26 @@ serde_json = "1.0" | |
| thiserror = "1.0" | ||
|
|
||
| [workspace.lints.clippy] | ||
|
There was a problem hiding this comment. i guess my question would be why are these lints not enabled by default? Do we want to enable so many? I usually follow the defaults for lints as there is sometimes a reason why they are not enabled yet. As otherwise this can become an explosion of debates. There was a problem hiding this comment. No clippy lints are enabled by default as far as I'm aware. The nice thing about making lints opt-out instead of opt-in is that there are a lot of lints and I think most of them are useful—I learned some new best practices in the course of making this PR, such as clippy::ptr_as_ptr. This PR has actually been something I've been working on for a while because it has been a useful exercise for me, and it has yielded other PRs in the process such as #1324 and #1321. So the effort was worthwhile in itself, even if this PR doesn't get merged. |
||
| all = { level = "warn", priority = -1 } | ||
| pedantic = { level = "warn", priority = -1 } | ||
| cast_possible_truncation = "allow" | ||
| cast_possible_wrap = "allow" | ||
| cast_sign_loss = "allow" | ||
| doc_link_with_quotes = "allow" | ||
| doc_markdown = "allow" | ||
| if_not_else = "allow" | ||
| incompatible_msrv = "deny" | ||
| inline_always = "allow" | ||
| manual_let_else = "allow" | ||
| many_single_char_names = "allow" | ||
| map_unwrap_or = "allow" | ||
| match_wildcard_for_single_variants = "allow" | ||
| module_name_repetitions = "allow" | ||
| must_use_candidate = "allow" | ||
| redundant_else = "allow" | ||
| return_self_not_must_use = "allow" | ||
| should_panic_without_expect = "allow" | ||
| similar_names = "allow" | ||
| single_match_else = "allow" | ||
| struct_excessive_bools = "allow" | ||
| too_many_lines = "allow" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,8 @@ | |
| // SPDX-FileContributor: Gerhard de Clercq <[email protected]> | ||
| // | ||
| // SPDX-License-Identifier: MIT OR Apache-2.0 | ||
| #![deny(missing_docs)] | ||
| #![allow(clippy::missing_panics_doc)] | ||
|
|
||
| //! This crate provides a builder which parses given Rust source code to search | ||
| //! for CXX-Qt or CXX macros and generate any resulting C++ code. It also builds | ||
|
|
@@ -115,24 +115,20 @@ impl GeneratedCpp { | |
| match item { | ||
| CxxQtItem::Cxx(m) => { | ||
| // TODO: later we will allow for multiple CXX or CXX-Qt blocks in one file | ||
| assert!(!found_bridge, | ||
| "Unfortunately only files with either a single cxx or a single cxx_qt module are currently supported. | ||
| The file {} has more than one of these.", | ||
| rust_file_path.display()); | ||
| found_bridge = true; | ||
|
|
||
| tokens.extend(m.into_token_stream()); | ||
| } | ||
| CxxQtItem::CxxQt(m) => { | ||
| // TODO: later we will allow for multiple CXX or CXX-Qt blocks in one file | ||
| assert!(!found_bridge, | ||
| "Unfortunately only files with either a single cxx or a single cxx_qt module are currently supported. | ||
| The file {} has more than one of these.", | ||
| rust_file_path.display()); | ||
| found_bridge = true; | ||
|
|
||
| let mut parser = Parser::from(*m.clone()) | ||
|
|
@@ -203,8 +199,7 @@ impl GeneratedCpp { | |
| let mut header = | ||
| File::create(&header_path).expect("Could not create cxx-qt header file"); | ||
| let header_generated = match cxx_qt_generated { | ||
| CppFragment::Header(header) | CppFragment::Pair { header, source: _ } => header, | ||
| CppFragment::Source(_) => panic!("Unexpected call for source fragment."), | ||
| }; | ||
| header | ||
|
|
@@ -223,9 +218,8 @@ impl GeneratedCpp { | |
| } | ||
| let mut cpp = File::create(&cpp_path).expect("Could not create cxx-qt source file"); | ||
| let source_generated = match cxx_qt_generated { | ||
| CppFragment::Source(source) | CppFragment::Pair { header: _, source } => source, | ||
| CppFragment::Header(_) => panic!("Unexpected call for header fragment."), | ||
| }; | ||
| cpp.write_all(source_generated.as_bytes()) | ||
| .expect("Could not write cxx-qt source file"); | ||
|
|
@@ -521,11 +515,9 @@ impl CxxQtBuilder { | |
| /// to disable any qt modules that are optional. | ||
| pub fn qt_module(mut self, module: &str) -> Self { | ||
| // Ensure that CMake and Cargo build.rs are not out of sync | ||
| assert!(qt_modules_import().is_none() || self.qt_modules.contains(module), "Qt module mismatch between cxx-qt-build and CMake!\n\ | ||
| Qt module '{module}' was not specified in CMake!\n\ | ||
| When building with CMake, all Qt modules must be specified with the QT_MODULES argument in cxx_qt_import_crate"); | ||
|
|
||
| self.qt_modules.insert(module.to_owned()); | ||
| self | ||
|
|
@@ -608,7 +600,7 @@ impl CxxQtBuilder { | |
| self | ||
| } | ||
|
|
||
| fn define_cfg_variable(key: &str, value: Option<&str>) { | ||
| if let Some(value) = value { | ||
| println!("cargo::rustc-cfg={key}=\"{value}\""); | ||
| } else { | ||
|
|
@@ -618,7 +610,7 @@ impl CxxQtBuilder { | |
| env::set_var(variable_cargo, value.unwrap_or("true")); | ||
| } | ||
|
|
||
| fn define_cfg_check_variable(key: &str, values: Option<Vec<&str>>) { | ||
| if let Some(values) = values { | ||
| let values = values | ||
| .iter() | ||
|
|
@@ -632,29 +624,26 @@ impl CxxQtBuilder { | |
| } | ||
| } | ||
|
|
||
| fn define_qt_version_cfg_variables(version: &Version) { | ||
| // Allow for Qt 5 or Qt 6 as valid values | ||
| CxxQtBuilder::define_cfg_check_variable("cxxqt_qt_version_major", Some(vec!["5", "6"])); | ||
| // Find the Qt version and tell the Rust compiler | ||
| // this allows us to have conditional Rust code | ||
| CxxQtBuilder::define_cfg_variable( | ||
| "cxxqt_qt_version_major", | ||
| Some(version.major.to_string().as_str()), | ||
| ); | ||
|
|
||
| // Seed all values from Qt 5.0 through to Qt 7.99 | ||
| for major in 5..=7 { | ||
| CxxQtBuilder::define_cfg_check_variable( | ||
| &format!("cxxqt_qt_version_at_least_{major}"), | ||
| None, | ||
| ); | ||
|
|
||
| for minor in 0..=99 { | ||
| CxxQtBuilder::define_cfg_check_variable( | ||
| &format!("cxxqt_qt_version_at_least_{major}_{minor}"), | ||
| None, | ||
| ); | ||
| } | ||
|
|
@@ -663,13 +652,13 @@ impl CxxQtBuilder { | |
| for minor in 0..=version.minor { | ||
| let qt_version_at_least = | ||
| format!("cxxqt_qt_version_at_least_{}_{}", version.major, minor); | ||
| CxxQtBuilder::define_cfg_variable(&qt_version_at_least, None); | ||
| } | ||
|
|
||
| // We don't support Qt < 5 | ||
| for major in 5..=version.major { | ||
| let at_least_qt_major_version = format!("cxxqt_qt_version_at_least_{major}"); | ||
| CxxQtBuilder::define_cfg_variable(&at_least_qt_major_version, None); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -689,7 +678,7 @@ impl CxxQtBuilder { | |
| // A dependency can specify which of its own include paths it wants to export. | ||
| // Set up each of these exported include paths as symlinks in our own include directory, | ||
| // or deep copy the files if the platform does not support symlinks. | ||
| fn include_dependency(dependency: &Dependency) { | ||
| let header_root = dir::header_root(); | ||
| let dependency_root = dependency.path.join("include"); | ||
| for include_prefix in &dependency.manifest.exported_include_prefixes { | ||
|
|
@@ -760,9 +749,9 @@ impl CxxQtBuilder { | |
| fn export_object_file( | ||
| mut obj_builder: cc::Build, | ||
| file_path: impl AsRef<Path>, | ||
| export_path: &Path, | ||
| ) { | ||
| obj_builder.file(file_path); | ||
|
|
||
| // We only expect a single file, so destructure the vec. | ||
| // If there's 0 or > 1 file, we panic in the `else` branch, because then the builder is | ||
|
|
@@ -777,7 +766,7 @@ impl CxxQtBuilder { | |
| ) | ||
| }); | ||
| } | ||
| std::fs::copy(obj_file, export_path).unwrap_or_else(|_| { | ||
| panic!("Failed to export object file to {}!", export_path.display()) | ||
| }); | ||
| } else { | ||
|
|
@@ -825,13 +814,12 @@ impl CxxQtBuilder { | |
| } | ||
| }) | ||
| .collect::<HashSet<String>>(); | ||
| assert!( | ||
| dirs.len() <= 1, | ||
| "Only one directory is supported per QmlModule for rust_files.\n\ | ||
| This is due to Qt bug https://bugreports.qt.io/browse/QTBUG-93443\n\ | ||
| Found directories: {dirs:?}" | ||
| ); | ||
|
|
||
| // TODO: for now we use the global CxxQtBuilder cc_builder | ||
| // this means that any includes/files etc on these are in this builder | ||
|
|
@@ -1035,7 +1023,7 @@ extern "C" bool {init_fun}() {{ | |
| std::fs::write(&init_file, init_call).expect("Could not write initializers call file!"); | ||
|
|
||
| if let Some(export_path) = export_path { | ||
| Self::export_object_file(init_call_builder, init_file, &export_path); | ||
| } else { | ||
| // Link the call-init-lib with +whole-archive to ensure that the static initializers are not discarded. | ||
| // We previously used object files that we linked directly into the final binary, but this caused | ||
|
|
@@ -1082,6 +1070,8 @@ extern "C" bool {init_fun}() {{ | |
| /// Generate and compile cxx-qt C++ code, as well as compile any additional files from | ||
| /// [CxxQtBuilder::qobject_header] and [CxxQtBuilder::cc_builder]. | ||
| pub fn build(mut self) -> Interface { | ||
| const MAX_INCLUDE_DEPTH: usize = 6; | ||
|
|
||
| dir::clean(dir::crate_target()).expect("Failed to clean crate export directory!"); | ||
|
|
||
| // We will do these two steps first, as setting up the dependencies can modify flags we | ||
|
|
@@ -1091,7 +1081,7 @@ extern "C" bool {init_fun}() {{ | |
| Self::write_common_headers(); | ||
| let dependencies = Dependency::find_all(); | ||
| for dependency in &dependencies { | ||
| Self::include_dependency(dependency); | ||
| } | ||
| let qt_modules = self.qt_modules(&dependencies); | ||
|
|
||
|
|
@@ -1103,7 +1093,7 @@ extern "C" bool {init_fun}() {{ | |
| let mut qtbuild = qt_build_utils::QtBuild::new(qt_modules.iter().cloned().collect()) | ||
| .expect("Could not find Qt installation"); | ||
| qtbuild.cargo_link_libraries(&mut self.cc_builder); | ||
| Self::define_qt_version_cfg_variables(&qtbuild.version()); | ||
|
|
||
| // Ensure that Qt modules and apple framework are linked and searched correctly | ||
| let mut include_paths = qtbuild.include_paths(); | ||
|
|
@@ -1115,7 +1105,6 @@ extern "C" bool {init_fun}() {{ | |
| // to the generated files without any namespacing. | ||
| include_paths.push(header_root.join(&self.include_prefix)); | ||
|
|
||
| let crate_header_dir = self.crate_include_root.as_ref().map(|subdir| { | ||
| dir::manifest() | ||
| .expect("Could not find crate directory!") | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we were testing in CI with our MSRV, do we not want to do this now?
Or instead should something like clippy be in it's own runner using a newer Rust toolchain 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, clippy could also just be separate runner, as I think it doesn't share many build artifacts with the main build anyhow.