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

Fix to never have undefined iteration order and lint against it #4665

Merged
merged 1 commit into from
Dec 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
3 changes: 3 additions & 0 deletions src/wasm-lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ http = "1"
kittycad = { version = "0.3.28", default-features = false, features = ["js", "requests"] }
kittycad-modeling-cmds = { version = "0.2.77", features = ["websocket"] }

[workspace.lints.clippy]
iter_over_hash_type = "warn"

[[test]]
name = "executor"
path = "tests/executor/main.rs"
Expand Down
3 changes: 3 additions & 0 deletions src/wasm-lib/derive-docs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@ anyhow = "1.0.93"
expectorate = "1.1.0"
pretty_assertions = "1.4.1"
rustfmt-wrapper = "0.2.1"

[lints]
workspace = true
3 changes: 3 additions & 0 deletions src/wasm-lib/kcl-test-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,6 @@ pico-args = "0.5.0"
serde = { version = "1.0.214", features = ["derive"] }
serde_json = "1.0.128"
tokio = { version = "1.41.1", features = ["macros", "rt-multi-thread"] }

[lints]
workspace = true
3 changes: 3 additions & 0 deletions src/wasm-lib/kcl-to-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ kittycad = { workspace = true, features = ["clap"] }
kittycad-modeling-cmds = { workspace = true }
tokio = { version = "1.41", features = ["full", "time", "rt", "tracing"] }
uuid = { version = "1.11.0", features = ["v4", "js", "serde"] }

[lints]
workspace = true
3 changes: 3 additions & 0 deletions src/wasm-lib/kcl/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ pretty_assertions = "1.4.1"
tokio = { version = "1.41.1", features = ["rt-multi-thread", "macros", "time"] }
twenty-twenty = "0.8.0"

[lints]
workspace = true

[[bench]]
name = "compiler_benchmark_criterion"
harness = false
Expand Down
3 changes: 3 additions & 0 deletions src/wasm-lib/kcl/fuzz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ members = ["."]
[profile.release]
debug = 1

[lints]
workspace = true

[[bin]]
name = "parser"
path = "fuzz_targets/parser.rs"
Expand Down
5 changes: 3 additions & 2 deletions src/wasm-lib/kcl/src/docs/gen_std_tests.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use std::collections::{BTreeMap, HashMap};
use std::collections::BTreeMap;

use anyhow::Result;
use base64::Engine;
use convert_case::Casing;
use handlebars::Renderable;
use indexmap::IndexMap;
use itertools::Itertools;
use serde_json::json;

Expand Down Expand Up @@ -271,7 +272,7 @@ fn init_handlebars() -> Result<handlebars::Handlebars<'static>> {
Ok(hbs)
}

fn generate_index(combined: &HashMap<String, Box<dyn StdLibFn>>) -> Result<()> {
fn generate_index(combined: &IndexMap<String, Box<dyn StdLibFn>>) -> Result<()> {
let hbs = init_handlebars()?;

let mut functions = Vec::new();
Expand Down
7 changes: 6 additions & 1 deletion src/wasm-lib/kcl/src/engine/conn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,12 @@ impl EngineConnection {
WebSocketResponse::Success(SuccessWebSocketResponse {
resp: OkWebSocketResponseData::ModelingBatch { responses },
..
}) => {
}) =>
{
#[expect(
clippy::iter_over_hash_type,
reason = "modeling command uses a HashMap and keys are random, so we don't really have a choice"
)]
for (resp_id, batch_response) in responses {
let id: uuid::Uuid = (*resp_id).into();
match batch_response {
Expand Down
4 changes: 4 additions & 0 deletions src/wasm-lib/kcl/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,10 @@ pub trait EngineManager: std::fmt::Debug + Send + Sync + 'static {
responses: HashMap<uuid::Uuid, BatchResponse>,
) -> Result<OkWebSocketResponseData, crate::errors::KclError> {
// Iterate over the responses and check for errors.
#[expect(
clippy::iter_over_hash_type,
reason = "modeling command uses a HashMap and keys are random, so we don't really have a choice"
)]
for (cmd_id, resp) in responses.iter() {
match resp {
BatchResponse::Success { response } => {
Expand Down
15 changes: 6 additions & 9 deletions src/wasm-lib/kcl/src/executor.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
//! The executor for the AST.

use std::{
collections::{HashMap, HashSet},
sync::Arc,
};
use std::{collections::HashSet, sync::Arc};

use anyhow::Result;
use async_recursion::async_recursion;
Expand Down Expand Up @@ -193,7 +190,7 @@ impl EnvironmentRef {

#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)]
pub struct Environment {
bindings: HashMap<String, KclValue>,
bindings: IndexMap<String, KclValue>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to prefer IndexMap over BTreeMap?

Copy link
Collaborator Author

@jtran jtran Dec 4, 2024

Choose a reason for hiding this comment

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

BTreeMap keeps things in sorted order according to the key type's Ord trait. IndexMap preserves insertion order. Unless we want to sort things, insertion order is a better, more intuitive default, in my opinion. For bindings specifically, it will keep variables in the order that they were defined in the KCL source. That's pretty nice.

parent: Option<EnvironmentRef>,
}

Expand All @@ -203,7 +200,7 @@ impl Environment {
pub fn root() -> Self {
Self {
// Prelude
bindings: HashMap::from([
bindings: IndexMap::from([
("ZERO".to_string(), KclValue::from_number(0.0, NO_META)),
("QUARTER_TURN".to_string(), KclValue::from_number(90.0, NO_META)),
("HALF_TURN".to_string(), KclValue::from_number(180.0, NO_META)),
Expand All @@ -215,7 +212,7 @@ impl Environment {

pub fn new(parent: EnvironmentRef) -> Self {
Self {
bindings: HashMap::new(),
bindings: IndexMap::new(),
parent: Some(parent),
}
}
Expand Down Expand Up @@ -770,8 +767,8 @@ pub struct Sketch {
/// The starting path.
pub start: BasePath,
/// Tag identifiers that have been declared in this sketch.
#[serde(default, skip_serializing_if = "HashMap::is_empty")]
pub tags: HashMap<String, TagIdentifier>,
#[serde(default, skip_serializing_if = "IndexMap::is_empty")]
pub tags: IndexMap<String, TagIdentifier>,
/// The original id of the sketch. This stays the same even if the sketch is
/// is sketched on face etc.
#[serde(skip)]
Expand Down
9 changes: 4 additions & 5 deletions src/wasm-lib/kcl/src/std/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,10 @@ pub mod types;
pub mod units;
pub mod utils;

use std::collections::HashMap;

use anyhow::Result;
pub use args::Args;
use derive_docs::stdlib;
use indexmap::IndexMap;
use lazy_static::lazy_static;
use parse_display::{Display, FromStr};
use schemars::JsonSchema;
Expand Down Expand Up @@ -167,8 +166,8 @@ pub fn get_stdlib_fn(name: &str) -> Option<Box<dyn StdLibFn>> {
}

pub struct StdLib {
pub fns: HashMap<String, Box<dyn StdLibFn>>,
pub kcl_fns: HashMap<String, Box<dyn KclStdLibFn>>,
pub fns: IndexMap<String, Box<dyn StdLibFn>>,
pub kcl_fns: IndexMap<String, Box<dyn KclStdLibFn>>,
}

impl std::fmt::Debug for StdLib {
Expand Down Expand Up @@ -198,7 +197,7 @@ impl StdLib {
}

// Get the combined hashmaps.
pub fn combined(&self) -> HashMap<String, Box<dyn StdLibFn>> {
pub fn combined(&self) -> IndexMap<String, Box<dyn StdLibFn>> {
let mut combined = self.fns.clone();
for (k, v) in self.kcl_fns.clone() {
combined.insert(k, v.std_lib());
Expand Down
5 changes: 2 additions & 3 deletions src/wasm-lib/kcl/src/std/sketch.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
//! Functions related to sketching.

use std::collections::HashMap;

use anyhow::Result;
use derive_docs::stdlib;
use indexmap::IndexMap;
use kcmc::shared::Point2d as KPoint2d; // Point2d is already defined in this pkg, to impl ts_rs traits.
use kcmc::{each_cmd as mcmd, length_unit::LengthUnit, shared::Angle, ModelingCmd};
use kittycad_modeling_cmds as kcmc;
Expand Down Expand Up @@ -1301,7 +1300,7 @@
}),
surface: None,
});
HashMap::from([(tag.name.to_string(), tag_identifier)])
IndexMap::from([(tag.name.to_string(), tag_identifier)])

Check warning on line 1303 in src/wasm-lib/kcl/src/std/sketch.rs

View check run for this annotation

Codecov / codecov/patch

src/wasm-lib/kcl/src/std/sketch.rs#L1303

Added line #L1303 was not covered by tests
} else {
Default::default()
},
Expand Down
Loading