ByoungSooPark/modeling-app
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

KCL: Fix cryptic error when using duplicate edges in fillet call (#5755)

Browse Source 
Fixes https://github.com/KittyCAD/modeling-app/issues/4307

Now if you try to fillet the same edge twice in a single fillet command,
the error message is clearer, and the source range will highlight
the specific edges in the array which are duplicated.

Same goes for chamfer.

Note: although the Rust KCL interpreter sends back an array of SourceRange
for each KCL error, the frontend only puts the first one into CodeMirror
diagnostics. We should fix that: https://github.com/KittyCAD/modeling-app/issues/5754
This commit is contained in:
Adam Chalmers
2025-03-12 11:24:27 -05:00
committed by GitHub
parent f8e53c6577
commit 865bf8ae7a
7 changed files with 125 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
rust/kcl-lib/src/std/args.rs
View File

@ -159,6 +159,49 @@ impl Args {
})
}
/// Get a labelled keyword arg, check it's an array, and return all items in the array
/// plus their source range.
pub(crate) fn kw_arg_array_and_source<'a, T>(&'a self, label: &str) -> Result<Vec<(T, SourceRange)>, KclError>
where
T: FromKclValue<'a>,
{
let Some(arg) = self.kw_args.labeled.get(label) else {
let err = KclError::Semantic(KclErrorDetails {
source_ranges: vec![self.source_range],
message: format!("This function requires a keyword argument '{label}'"),
});
return Err(err);
};
let Some(array) = arg.value.as_array() else {
let err = KclError::Semantic(KclErrorDetails {
source_ranges: vec![arg.source_range],
message: format!(
"Expected an array of {} but found {}",
type_name::<T>(),
arg.value.human_friendly_type()
),
});
return Err(err);
};
array
.iter()
.map(|item| {
let source = SourceRange::from(item);
let val = FromKclValue::from_kcl_val(item).ok_or_else(|| {
KclError::Semantic(KclErrorDetails {
source_ranges: arg.source_ranges(),
message: format!(
"Expected a {} but found {}",
type_name::<T>(),
arg.value.human_friendly_type()
),
})
})?;
Ok((val, source))
})
.collect::<Result<Vec<_>, _>>()
}
/// Get the unlabeled keyword argument. If not set, returns None.
pub(crate) fn unlabeled_kw_arg_unconverted(&self) -> Option<&Arg> {
self.kw_args

14
rust/kcl-lib/src/std/chamfer.rs
View File

@ -5,7 +5,6 @@ use kcl_derive_docs::stdlib;
use kcmc::{each_cmd as mcmd, length_unit::LengthUnit, shared::CutType, ModelingCmd};
use kittycad_modeling_cmds as kcmc;
use super::utils::unique_count;
use crate::{
errors::{KclError, KclErrorDetails},
execution::{ChamferSurface, EdgeCut, ExecState, ExtrudeSurface, GeoMeta, KclValue, Solid},
@ -19,9 +18,11 @@ pub(crate) const DEFAULT_TOLERANCE: f64 = 0.0000001;
pub async fn chamfer(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
let solid = args.get_unlabeled_kw_arg("solid")?;
let length = args.get_kw_arg("length")?;
let tags = args.get_kw_arg("tags")?;
let tags = args.kw_arg_array_and_source::<EdgeReference>("tags")?;
let tag = args.get_kw_arg_opt("tag")?;
super::fillet::validate_unique(&tags)?;
let tags: Vec<EdgeReference> = tags.into_iter().map(|item| item.0).collect();
let value = inner_chamfer(solid, length, tags, tag, exec_state, args).await?;
Ok(KclValue::Solid { value })
}
@ -109,15 +110,6 @@ async fn inner_chamfer(
exec_state: &mut ExecState,
args: Args,
) -> Result<Box<Solid>, KclError> {
// Check if tags contains any duplicate values.
let unique_tags = unique_count(tags.clone());
if unique_tags != tags.len() {
return Err(KclError::Type(KclErrorDetails {
message: "Duplicate tags are not allowed.".to_string(),
source_ranges: vec![args.source_range],
}));
}
// If you try and tag multiple edges with a tagged chamfer, we want to return an
// error to the user that they can only tag one edge at a time.
if tag.is_some() && tags.len() > 1 {

59
rust/kcl-lib/src/std/fillet.rs
View File

@ -1,6 +1,7 @@
//! Standard library fillets.
use anyhow::Result;
use indexmap::IndexMap;
use kcl_derive_docs::stdlib;
use kcmc::{
each_cmd as mcmd, length_unit::LengthUnit, ok_response::OkModelingCmdResponse, shared::CutType,
@ -11,13 +12,13 @@ use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use uuid::Uuid;
use super::utils::unique_count;
use crate::{
errors::{KclError, KclErrorDetails},
execution::{EdgeCut, ExecState, ExtrudeSurface, FilletSurface, GeoMeta, KclValue, Solid, TagIdentifier},
parsing::ast::types::TagNode,
settings::types::UnitLength,
std::Args,
SourceRange,
};
/// A tag or a uuid of an edge.
@ -40,13 +41,39 @@ impl EdgeReference {
}
}
pub(super) fn validate_unique<T: Eq + std::hash::Hash>(tags: &[(T, SourceRange)]) -> Result<(), KclError> {
// Check if tags contains any duplicate values.
let mut tag_counts: IndexMap<&T, Vec<SourceRange>> = Default::default();
for tag in tags {
tag_counts.entry(&tag.0).or_insert(Vec::new()).push(tag.1);
}
let mut duplicate_tags_source = Vec::new();
for (_tag, count) in tag_counts {
if count.len() > 1 {
duplicate_tags_source.extend(count)
}
}
if !duplicate_tags_source.is_empty() {
return Err(KclError::Type(KclErrorDetails {
message: "The same edge ID is being referenced multiple times, which is not allowed. Please select a different edge".to_string(),
source_ranges: duplicate_tags_source,
}));
}
Ok(())
}
/// Create fillets on tagged paths.
pub async fn fillet(exec_state: &mut ExecState, args: Args) -> Result<KclValue, KclError> {
// Get all args:
let solid = args.get_unlabeled_kw_arg("solid")?;
let radius = args.get_kw_arg("radius")?;
let tolerance = args.get_kw_arg_opt("tolerance")?;
let tags = args.get_kw_arg("tags")?;
let tags = args.kw_arg_array_and_source::<EdgeReference>("tags")?;
let tag = args.get_kw_arg_opt("tag")?;
// Run the function.
validate_unique(&tags)?;
let tags: Vec<EdgeReference> = tags.into_iter().map(|item| item.0).collect();
let value = inner_fillet(solid, radius, tags, tolerance, tag, exec_state, args).await?;
Ok(KclValue::Solid { value })
}
@ -129,15 +156,6 @@ async fn inner_fillet(
exec_state: &mut ExecState,
args: Args,
) -> Result<Box<Solid>, KclError> {
// Check if tags contains any duplicate values.
let unique_tags = unique_count(tags.clone());
if unique_tags != tags.len() {
return Err(KclError::Type(KclErrorDetails {
message: "Duplicate tags are not allowed.".to_string(),
source_ranges: vec![args.source_range],
}));
}
let mut solid = solid.clone();
for edge_tag in tags {
let edge_id = edge_tag.get_engine_id(exec_state, &args)?;
@ -432,3 +450,22 @@ pub(crate) fn default_tolerance(units: &UnitLength) -> f64 {
UnitLength::M => 0.001,
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_validate_unique() {
let dup_a = SourceRange::from([1, 3, 0]);
let dup_b = SourceRange::from([10, 30, 0]);
// Two entries are duplicates (abc) with different source ranges.
let tags = vec![("abc", dup_a), ("abc", dup_b), ("def", SourceRange::from([2, 4, 0]))];
let actual = validate_unique(&tags);
// Both the duplicates should show up as errors, with both of the
// source ranges they correspond to.
// But the unique source range 'def' should not.
let expected = vec![dup_a, dup_b];
assert_eq!(actual.err().unwrap().source_ranges(), expected);
}
}

17
rust/kcl-lib/src/std/utils.rs
View File

@ -1,4 +1,4 @@
use std::{collections::HashSet, f64::consts::PI};
use std::f64::consts::PI;
use kittycad_modeling_cmds::shared::Angle;
@ -8,16 +8,6 @@ use crate::{
source_range::SourceRange,
};
/// Count the number of unique items in a `Vec` in O(n) time.
pub(crate) fn unique_count<T: Eq + std::hash::Hash>(vec: Vec<T>) -> usize {
// Add to a set.
let mut set = HashSet::with_capacity(vec.len());
for item in vec {
set.insert(item);
}
set.len()
}
/// Get the distance between two points.
pub fn distance(a: Point2d, b: Point2d) -> f64 {
((b.x - a.x).powi(2) + (b.y - a.y).powi(2)).sqrt()
@ -686,11 +676,6 @@ mod get_tangential_arc_to_info_tests {
(num * 1000.0).round() / 1000.0
}
#[test]
fn test_unique_count() {
assert_eq!(unique_count(vec![1, 2, 2, 3, 2]), 3);
}
#[test]
fn test_basic_case() {
let result = get_tangential_arc_to_info(TangentialArcInfoInput {