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

Import our intended set of lints #432

Merged
merged 14 commits into from
Jul 18, 2024
65 changes: 64 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ repository = "https://github.com/linebender/xilem"
homepage = "https://xilem.dev/"

[workspace.lints]
clippy.semicolon_if_nothing_returned = "warn"
# Remove assigning_clones once it's allowed by default in stable Rust
# https://github.com/rust-lang/rust-clippy/pull/12779
clippy.assigning_clones = "allow"
Expand All @@ -37,6 +36,70 @@ rust.unexpected_cfgs = { level = "warn", check-cfg = [
# (And cargo doesn't let us have platform specific lints here)
rust.unsafe_code = "deny"

rust.keyword_idents_2024 = "forbid"
rust.non_ascii_idents = "forbid"
rust.unsafe_op_in_unsafe_fn = "forbid"
rust.non_local_definitions = "forbid"

rust.unused_lifetimes = "warn"
rust.unit_bindings = "warn"
rust.unused_import_braces = "warn"
rust.trivial_numeric_casts = "warn"
rust.unused_macro_rules = "warn"
rust.variant_size_differences = "warn"

clippy.allow_attributes_without_reason = "warn"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this, pollutes my diagnostics in my editor, without being able to action about it, since this does seem to be experimental, and thus CI fails when adding a reason (as seen here) , should we disable that for now, and enable it, when it's stable, maybe with directly fixing all of this, so that it doesn't contribute to OCD^^?

Copy link
Member Author

@DJMcNab DJMcNab Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can comment this out.

However, the behaviour you're seeing is confusing - since rust-lang/rust-clippy#12999, this shouldn't be happening for you, because it checks the MSRV to see whether to suggest this lint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's interesting (just updated nightly, and still the issue).
I don't use rust via rustup, but via rust-overlay maybe that's an issue, as there's no rustup-binary on my system. I mean it's not critical to me, I can just comment it out locally.

clippy.collection_is_never_read = "warn"
clippy.debug_assert_with_mut_call = "warn"
clippy.fn_to_numeric_cast_any = "forbid"
clippy.infinite_loop = "warn"
clippy.large_include_file = "warn"
clippy.large_stack_arrays = "warn"
clippy.mismatching_type_param_order = "warn"
clippy.missing_fields_in_debug = "warn"
clippy.same_functions_in_if_condition = "warn"
clippy.semicolon_if_nothing_returned = "warn"
clippy.should_panic_without_expect = "warn"
clippy.unseparated_literal_suffix = "warn"

# Follow-ups for their own PRs, too noisy to go in lint group PR

# rust.let_underscore_drop = "warn"
# rust.missing_debug_implementations = "warn"
# rust.unused_qualifications = "warn"
# rust.single_use_lifetimes = "warn"
# clippy.exhaustive_enums = "warn"
# clippy.dbg_macro = "warn"
# clippy.match_same_arms = "warn"
# clippy.cast_possible_truncation = "warn"
# clippy.missing_assert_message = "warn"
# clippy.return_self_not_must_use = "warn"
# clippy.wildcard_imports = "warn"
# rust.elided_lifetimes_in_paths = "warn"
# clippy.use_self = "warn"

# Aspirational lints, not enabled for one reason or another

# rust.missing_docs = "warn" # We have many as-yet undocumented items
# rust.unreachable_pub = "warn" # Potentially controversial code style
# rust.unnameable_types = "warn" # Requires lint_reasons rustc feature for exceptions
# clippy.todo = "warn" # We have a lot of "real" todos
# clippy.missing_errors_doc = "warn" # Can be quite noisy?
# clippy.missing_panics_doc = "warn" # Can be quite noisy?
# clippy.partial_pub_fields = "warn" # Potentially controversial code style
# clippy.shadow_unrelated = "warn" # Potentially controversial code style

# This catches duplicated dependencies in the tree, which we don't have much control over
# We should use cargo deny for this, anyway
# clippy.cargo = "warn"

# Lints which we still set in individual crates lib.rs
# False positives with example targets - https://github.com/rust-lang/rust/issues/57274
# rust.unused_crate_dependencies = "warn"
# Examples often do want to print
# clippy.print_stdout = "warn" # Note that this is allowed in Masonry
# clippy.print_stderr = "warn" # Note that this is allowed in Masonry

[workspace.dependencies]
xilem_web_core = { version = "0.1.0", path = "xilem_web/xilem_web_core" }
masonry = { version = "0.2.0", path = "masonry" }
Expand Down
2 changes: 1 addition & 1 deletion masonry/examples/calc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

// On Windows platform, don't show a console when opening the app.
#![windows_subsystem = "windows"]
#![allow(clippy::single_match)]
#![allow(variant_size_differences, clippy::single_match)]

use accesskit::{DefaultActionVerb, Role};
use masonry::app_driver::{AppDriver, DriverCtx};
Expand Down
4 changes: 3 additions & 1 deletion masonry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,13 @@
#![deny(clippy::trivially_copy_pass_by_ref)]
// #![deny(rustdoc::broken_intra_doc_links)]
// #![warn(missing_docs)]
#![warn(unused_imports)]
#![warn(unused_imports, /* TODO: clippy::print_stdout, clippy::print_stderr */)]
#![allow(clippy::should_implement_trait)]
#![allow(clippy::single_match)]
#![cfg_attr(docsrs, feature(doc_cfg))]
#![cfg_attr(not(debug_assertions), allow(unused))]
// False-positive with dev-dependencies only used in examples
#![cfg_attr(not(test), warn(unused_crate_dependencies))]

// TODO - Add logo

Expand Down
4 changes: 2 additions & 2 deletions masonry/src/promise.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ mod tests {
assert!(promise_result.try_get(promise_token_2).is_none());
}

#[should_panic]
#[should_panic(expected = "mismatched token")]
#[test]
fn bad_promise_get() {
let promise_token_1: PromiseToken<i32> = PromiseToken::new();
Expand All @@ -149,7 +149,7 @@ mod tests {
promise_result.get(promise_token_2);
}

#[should_panic]
#[should_panic(expected = "payload already taken")]
#[test]
fn get_promise_twice() {
let promise_token = PromiseToken::new();
Expand Down
3 changes: 2 additions & 1 deletion masonry/src/text/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,8 @@ impl<T: TextStorage> std::fmt::Debug for TextLayout<T> {
.field("outdated?", &self.needs_rebuild())
.field("width", &self.layout.width())
.field("height", &self.layout.height())
.finish()
.field("links", &self.links)
.finish_non_exhaustive()
}
}

Expand Down
2 changes: 1 addition & 1 deletion masonry/src/text/selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ pub trait EditableTextCursor {
impl<Str: Deref<Target = str> + TextStorage> Selectable for Str {
type Cursor<'a> = StringCursor<'a> where Self: 'a;

fn cursor<'a>(&self, position: usize) -> Option<StringCursor> {
fn cursor(&self, position: usize) -> Option<StringCursor> {
let new_cursor = StringCursor {
text: self,
position,
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/theme.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub const TEXTBOX_INSETS: Insets = Insets::new(4.0, 4.0, 4.0, 4.0);
pub const SCROLLBAR_COLOR: Color = Color::rgb8(0xff, 0xff, 0xff);
pub const SCROLLBAR_BORDER_COLOR: Color = Color::rgb8(0x77, 0x77, 0x77);
pub const SCROLLBAR_MAX_OPACITY: f64 = 0.7;
pub const SCROLLBAR_FADE_DELAY: u64 = 1500u64;
pub const SCROLLBAR_FADE_DELAY: u64 = 1500;
pub const SCROLLBAR_WIDTH: f64 = 8.;
pub const SCROLLBAR_PAD: f64 = 2.;
pub const SCROLLBAR_MIN_SIZE: f64 = 45.;
Expand Down
4 changes: 1 addition & 3 deletions masonry/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,13 @@ use std::hash::Hash;
///
/// It's useful when a backtrace would aid debugging but a crash can be avoided in release.
macro_rules! debug_panic {
() => { ... };
($msg:expr) => {
($msg:expr$(,)?) => {
if cfg!(debug_assertions) {
panic!($msg);
} else {
tracing::error!($msg);
}
};
($msg:expr,) => { debug_panic!($msg) };
($fmt:expr, $($arg:tt)+) => {
if cfg!(debug_assertions) {
panic!($fmt, $($arg)*);
Expand Down
2 changes: 1 addition & 1 deletion masonry/src/widget/align.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ impl Widget for Align {
ctx.set_paint_insets(my_insets);
if self.height_factor.is_some() {
let baseline_offset = ctx.child_baseline_offset(&self.child);
if baseline_offset > 0f64 {
if baseline_offset > 0_f64 {
ctx.set_baseline_offset(baseline_offset + extra_height / 2.0);
}
}
Expand Down
6 changes: 3 additions & 3 deletions masonry/src/widget/flex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -516,8 +516,8 @@ impl Widget for Flex {
// minor-axis values for all children
let mut minor = self.direction.minor(bc.min());
// these two are calculated but only used if we're baseline aligned
let mut max_above_baseline = 0f64;
let mut max_below_baseline = 0f64;
let mut max_above_baseline = 0_f64;
let mut max_below_baseline = 0_f64;
let mut any_use_baseline = self.cross_alignment == CrossAxisAlignment::Baseline;

// Measure non-flex children.
Expand Down Expand Up @@ -1085,7 +1085,7 @@ mod tests {

// TODO - fix this test
#[test]
#[should_panic]
#[ignore = "Unclear what test is trying to validate"]
fn test_invalid_flex_params() {
use float_cmp::approx_eq;
let params = FlexParams::new(0.0, None);
Expand Down
4 changes: 3 additions & 1 deletion xilem/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ tracing.workspace = true
vello.workspace = true
smallvec.workspace = true
accesskit.workspace = true
accesskit_winit.workspace = true
tokio = { version = "1.38.0", features = ["rt", "rt-multi-thread", "time"] }

[dev-dependencies]
accesskit_winit.workspace = true

[target.'cfg(target_os = "android")'.dev-dependencies]
winit = { features = ["android-native-activity"], workspace = true }
2 changes: 1 addition & 1 deletion xilem/src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ where
&mut self.state,
)
} else {
eprintln!("Got action {action:?} for unknown widget. Did you forget to use `with_action_widget`?");
tracing::error!("Got action {action:?} for unknown widget. Did you forget to use `with_action_widget`?");
return;
};
let rebuild = match message_result {
Expand Down
4 changes: 3 additions & 1 deletion xilem/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright 2024 the Xilem Authors
// SPDX-License-Identifier: Apache-2.0

#![allow(clippy::comparison_chain)]
// False-positive with dev-dependencies only used in examples
#![cfg_attr(not(test), warn(unused_crate_dependencies))]
#![warn(unnameable_types, unreachable_pub)]
#![warn(clippy::print_stdout, clippy::print_stderr)]
use std::{collections::HashMap, sync::Arc};

use masonry::{
Expand Down
8 changes: 7 additions & 1 deletion xilem_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@

#![cfg_attr(not(test), no_std)]
#![forbid(unsafe_code)]
#![warn(missing_docs, unreachable_pub)]
#![warn(
missing_docs,
unreachable_pub,
unused_crate_dependencies,
clippy::print_stdout,
clippy::print_stderr
)]
// https://linebender.org/blog/doc-include
//! <!-- This license link is in a .rustdoc-hidden section, but we may as well give the correct link -->
//! [LICENSE]: https://github.com/linebender/xilem/blob/main/xilem_core/LICENSE
Expand Down
3 changes: 1 addition & 2 deletions xilem_core/src/views/memoize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ It's not possible in Rust currently to check whether the (content of the) callba

/// Create a new `Memoize` view.
pub fn new(data: D, child_cb: F) -> Self {
#[allow(clippy::let_unit_value)]
let _ = Self::ASSERT_CONTEXTLESS_FN;
let () = Self::ASSERT_CONTEXTLESS_FN;
Memoize { data, child_cb }
}
}
Expand Down
2 changes: 1 addition & 1 deletion xilem_web/src/element_props.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub struct ElementProps {
pub(crate) attributes: Option<Box<Attributes>>,
pub(crate) classes: Option<Box<Classes>>,
pub(crate) styles: Option<Box<Styles>>,
pub children: Vec<AnyPod>,
pub(crate) children: Vec<AnyPod>,
}

impl ElementProps {
Expand Down
2 changes: 1 addition & 1 deletion xilem_web/web_examples/todomvc/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl AppState {
}
let title = self.new_todo.trim().to_string();
self.new_todo.clear();
let id = self.next_id();
let id: u64 = self.next_id();
self.todos.push(Todo::new(title, id));
self.focus_new_todo = true;
self.save();
Expand Down