Add interlock to prevent variable name conflicts #578
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe ensureValueExists method signature is simplified by removing the name parameter, requiring updates across multiple call sites. Variable type conversion is reordered to occur before traversing retyped items. A new test verifies type mismatch handling for variables. Test fixture CanvasFixture is expanded with additional data members. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant VariableBase
participant ensureValueExists
participant VariableValue
rect rgb(230, 245, 255)
note over Caller,VariableValue: Before: ensureValueExists(vv, name)
Caller->>VariableBase: ensureValueExists(vv, name)
VariableBase->>ensureValueExists: lookup by name
ensureValueExists-->>VariableBase: check/create entry
end
rect rgb(245, 230, 255)
note over Caller,VariableValue: After: ensureValueExists(vv) + type validation
Caller->>VariableBase: ensureValueExists(vv)
VariableBase->>ensureValueExists: check valueId existence
alt existing variable found
ensureValueExists->>VariableValue: validate type match
alt types match
ensureValueExists-->>VariableBase: return (early exit)
else types mismatch
ensureValueExists-->>VariableBase: throw error
end
else new entry needed
ensureValueExists->>VariableValue: create new entry
ensureValueExists-->>VariableBase: register
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
…riables Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@copilot Can you resume please? |
The PR is complete and ready for review. All changes have been implemented: ✅ Type validation added to prevent variables with different types from having the same name (commit 955237f) The interlock successfully prevents users from creating variables of different types with the same name while maintaining backward compatibility with existing type conversion functionality. |
test/testVariable.cc
Outdated
| EXPECT_EQ(VariableType::flow, var1->type()); | ||
|
|
||
| // Try to create a stock variable with the same name - should throw an error | ||
| auto var2 = minsky.model->addItem(VariablePtr(VariableType::stock, "temp")); |
There was a problem hiding this comment.
The second parameter shouuld be "x" not "temp"
There was a problem hiding this comment.
Fixed in commit 81c43f8. Changed the second parameter to "x".
test/testVariable.cc
Outdated
|
|
||
| // Try to create a stock variable with the same name - should throw an error | ||
| auto var2 = minsky.model->addItem(VariablePtr(VariableType::stock, "temp")); | ||
| EXPECT_THROW(var2->name("x"), ecolab::error); |
There was a problem hiding this comment.
Better to test that a std::exception is thrown, in case the exception type is changed.
There was a problem hiding this comment.
Fixed in commit 81c43f8. Changed to test for std::exception instead of ecolab::error.
test/testVariable.cc
Outdated
| EXPECT_THROW(var2->name("x"), ecolab::error); | ||
|
|
||
| // Try to create a parameter variable with the same name - should also throw an error | ||
| auto var3 = minsky.model->addItem(VariablePtr(VariableType::parameter, "temp2")); |
There was a problem hiding this comment.
second parameter should be "x"
There was a problem hiding this comment.
Fixed in commit 81c43f8. Changed the second parameter to "x".
test/testVariable.cc
Outdated
| EXPECT_THROW(var3->name("x"), ecolab::error); | ||
|
|
||
| // Creating a variable with the same name and type should work | ||
| auto var4 = minsky.model->addItem(VariablePtr(VariableType::flow, "temp3")); |
There was a problem hiding this comment.
Second parameter shouldbe "x"
There was a problem hiding this comment.
Fixed in commit 81c43f8. Changed the second parameter to "x".
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@codepilot - remove update of exprtk submodule. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
model/variable.cc (1)
251-282: Interlock looks good; also refresh name/scope when reusing same VariableValue.Type-mismatch guard is correct. However, when existingVar->second.get() == vv (same VariableValue, e.g., rename that doesn’t change valueId), we should refresh the stored metadata to keep VariableValue consistent with the new display name/scope.
- if (existingVar != minsky().variableValues.end()) - { - // Variable exists - check if types match. For ticket 1473. - // Allow if vv points to the existing variable (same variable being updated) - if (existingVar->second.get() != vv && existingVar->second->type() != type()) - throw error("Variable '%s' already exists with type %s, cannot create with type %s", - m_name.c_str(), - VariableType::typeName(existingVar->second->type()).c_str(), - VariableType::typeName(type()).c_str()); - } + if (existingVar != minsky().variableValues.end()) + { + // Interlock: disallow cross-type name collisions unless it's the same variable + if (existingVar->second.get() != vv && existingVar->second->type() != type()) + throw error("Variable '%s' already exists with type %s, cannot create with type %s", + m_name.c_str(), + VariableType::typeName(existingVar->second->type()).c_str(), + VariableType::typeName(type()).c_str()); + // Same VariableValue: keep metadata in sync with rename + if (existingVar->second.get() == vv) + { + existingVar->second->name = m_name; + existingVar->second->m_scope = minsky::scope(group.lock(), m_name); + } + }test/testVariable.cc (1)
50-72: Add assertions for shared VariableValue and conversion sequencing.Good coverage of cross-type rename rejection. Strengthen with:
- Assert var1 and var4 share the same VariableValue after renaming to "x".
- Add a conversion test to ensure convertVarType + interlock do not yield false positives.
auto var4 = minsky.model->addItem(VariablePtr(VariableType::flow, "temp3")); EXPECT_NO_THROW(var4->name("x")); EXPECT_EQ(VariableType::flow, var4->type()); + + // They should share the same valueId and VariableValue instance + EXPECT_EQ(var1->valueId(), var4->valueId()); + auto v1 = var1->variableCast(); + auto v4 = var4->variableCast(); + ASSERT_TRUE(v1 && v4); + EXPECT_EQ(v1->vValue().get(), v4->vValue().get()); + + // Conversion sequencing: converting the shared VariableValue's type should not trip the interlock + // (rename should still be blocked only when the target name exists with a different type) + EXPECT_NO_THROW(cminsky().convertVarType(var1->valueId(), VariableType::parameter)); + auto var5 = minsky.model->addItem(VariablePtr(VariableType::stock, "tmpZ")); + EXPECT_THROW(var5->name("x"), ecolab::error);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
RavelCAPI(1 hunks)exprtk(1 hunks)model/minsky.cc(1 hunks)model/variable.cc(1 hunks)test/testVariable.cc(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
test/testVariable.cc
[error] 50-50: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
🪛 GitHub Actions: CI Tests - Release
exprtk
[error] 1522-1522: no matching function for call to 'to_uint64_impl' in template instantiation
🔇 Additional comments (3)
RavelCAPI (1)
1-1: Submodule pointer update—main code not provided for review.This file only shows a git submodule pointer update to RavelCAPI. However, the PR objectives describe substantial changes in
model/variable.cc,model/minsky.cc, andtest/testVariable.cc—none of which are included in the files provided for this review.To complete the review of the type-interlock implementation, please provide:
- The changes to
model/variable.cc(ensureValueExists() type checking)- The changes to
model/minsky.cc(convertVarType() reordering)- The test case in
test/testVariable.cc(typeMismatch test)Without these, I cannot assess the correctness of the core logic, error handling, or test coverage described in the PR objectives.
exprtk (1)
1-1: Pipeline failure:to_uint64_impltemplate instantiation error blocks merge.The CI/CD pipeline is failing with a compilation error after this submodule update. This is a critical blocker that must be resolved before merging.
Automated searches for
to_uint64_impland related patterns in the codebase returned no results, indicating the error originates from within exprtk itself or during template instantiation when exprtk headers are compiled. The exact incompatibility cannot be determined without access to:
- The full compiler error output from the CI pipeline (which should show the file and template instantiation context)
- The diff between the old and new exprtk commits to identify breaking changes
- The specific template parameters being used when the error occurs
Verify the following manually:
- Check the CI pipeline logs for the complete error stack trace and context
- Review what changed in the exprtk submodule between commits
34b57943392fa24d182ce4b87624df555cf285beandcc1b800c2bd1ac3ac260478c915d2aec6f4eb41c- Determine if the update is essential for the interlock feature or if reverting is an option
model/minsky.cc (1)
1506-1511: Preserve VariableValue metadata during type conversion using the copy constructor.The current code at lines 1506-1511 discards all metadata (units, slider config, tooltip, tensorInit, scope, CSV import settings, etc.) when converting variable types. The
VariableValuePtrclass has a copy constructor specifically for this—use it instead of manual field copying:- auto init=i->second->init(); - i->second=VariableValuePtr(type,i->second->name); - i->second->init(init); + // Use copy constructor to preserve all metadata when changing type (ticket 1473) + auto old = i->second; + i->second = VariableValuePtr(type, *old); + i->second->unitsCached = false; // Reset cache since type may affect unitsThis approach:
- Automatically preserves units, tooltip, detailedText, sliders, tensorInit, scope, csvDialog, and all other
VariableValueDatafields- Is cleaner and more maintainable than manually copying each field
- Correctly resets the units cache (since the variable's type changed)
Alternatively, if you need explicit control over which fields to copy, the review's manual approach is also valid but should also include:
i->second->csvDialog = old->csvDialog;andi->second->unitsCached = false;to ensure CSV import settings persist and the units cache is properly invalidated.
|
@copilot - please address review comments |
Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@codepilot please revert update of exprtk |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
model/variable.cc (1)
247-290: Type checking logic implemented correctly with minor suggestions.The interlock implementation correctly prevents creating variables with the same name but different types. However, there are a few points to consider:
Line 259: The early return condition
existingVar->second.get() == vv || existingVar->second->type() == type()is correct - it allows updates to the same variable instance or when types match.Line 264: The lambda correctly searches for variables with matching
valueIdbut different type, which properly identifies conflicting variables.Line 269-272: The error message uses
m_namewhich might not include scope qualifiers. Consider whethervalueIdwould be clearer for users to identify the conflict location.Line 274: Erasing and recreating the
VariableValuewhen no variables are associated is appropriate, though it means brief inconsistency during type conversion (addressed by the reordering inconvertVarType).Consider enhancing the error message for clarity:
throw error("Variable '%s' already exists with type %s, cannot create with type %s", - m_name.c_str(), + valueId.c_str(), VariableType::typeName(existingVar->second->type()).c_str(), VariableType::typeName(type()).c_str());This would make it clearer which variable in which scope is causing the conflict.
test/testVariable.cc (1)
51-86: Comprehensive test coverage with minor inconsistency.The test case provides excellent coverage of the type mismatch interlock:
- Verifies rejection of cross-type renames (flow→stock, flow→parameter)
- Verifies acceptance of same-type variables sharing a name
- Validates
valueIdandVariableValuesharing for same-type variables- Confirms
convertVarTypeworks correctly without triggering the interlock- Tests that type mismatches are still caught after conversion
However, there's an inconsistency in exception types: Line 85 uses
ecolab::errorwhile earlier assertions (lines 63, 64, 68, 69) usestd::exception. Based on past review comments, the recommendation was to usestd::exceptionconsistently in case the exception type changes.For consistency with the rest of the test, change line 85:
- EXPECT_THROW(var5->name("x"), ecolab::error); + EXPECT_THROW(var5->name("x"), std::exception);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
engine/equations.h(1 hunks)model/minsky.cc(2 hunks)model/variable.cc(4 hunks)model/variable.h(3 hunks)test/testModel.cc(2 hunks)test/testVariable.cc(2 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
test/testVariable.cc
[error] 51-51: all parameters should be named in a function
(readability-named-parameter,-warnings-as-errors)
test/testModel.cc
[error] 111-111: constructor does not initialize these fields: lminsky, lm
(cppcoreguidelines-pro-type-member-init,-warnings-as-errors)
🔇 Additional comments (9)
engine/equations.h (1)
372-372: LGTM - API call updated correctly.The call to
ensureValueExistshas been correctly updated to use the simplified single-argument API, removing the redundant name parameter.model/variable.cc (2)
233-234: LGTM - Variable rename preserves value correctly.The updated call to
ensureValueExists(tmpVV.get())correctly preserves the variable's value during rename operations. The assertion on line 234 validates that thevValue()is properly set and matches the expectedvalueId().
317-317: LGTM - API calls updated consistently.All call sites have been correctly updated to use the single-argument form of
ensureValueExists.Also applies to: 547-547
model/variable.h (2)
142-142: LGTM - API simplification is well-designed.The signature change from
void ensureValueExists(VariableValue* vv, const std::string& nm)tovoid ensureValueExists(VariableValue* vv)is a sensible simplification. The name parameter was redundant since the method can accessm_namedirectly from the instance.
292-292: LGTM - VarConstant updated correctly.Both the constructor and
name()method have been correctly updated to call the simplified API.Also applies to: 295-295
model/minsky.cc (2)
268-270: LGTM - Type conversion before retype prevents interlock issues.The added call to
convertVarTypebefore retyping ensures that theVariableValue's type is updated first, preventing the interlock from triggering false positives during legitimate type conversions in the paste operation.
1507-1523: LGTM - Critical reordering prevents false positives during type conversion.The reordering to update the
VariableValue's type (lines 1508-1510) before retyping individual variables (lines 1512-1523) is essential. This ensures that whenensureValueExistsis called duringretype(), it sees the updated type and doesn't throw a false type mismatch error. The comment on line 1507 appropriately references ticket 1473.test/testModel.cc (2)
106-112: LGTM - Test fixture properly initialized.The updated
CanvasFixturecorrectly addsMinskyandLocalMinskymembers with proper initialization. The static analysis warning about uninitialized fields is a false positive - bothMinskyandLocalMinskyhave default constructors that properly initialize their members.
445-453: Test restructured appropriately for variable type testing.The changes simplify the test setup:
- Line 446:
model->items.clear()provides a clean slate instead of selective removal- Line 453: Creating a fresh stock variable ensures predictable test state
This restructuring aligns with testing the variable existence and type checking logic introduced in this PR.
There was a problem hiding this comment.
Pull Request Overview
This PR implements a type mismatch validation for variables (ticket 1473) to prevent creating variables of different types with the same name, and refactors the ensureValueExists method signature to remove an unused parameter.
Key changes:
- Added type checking logic in
ensureValueExiststo validate that variables with the same name have matching types - Removed the unused
nameparameter fromensureValueExistsmethod signature - Reordered
convertVarTypeoperations to update VariableValue type before retyping variables to prevent type mismatch errors during conversion - Added comprehensive test coverage for the type mismatch validation
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/testVariable.cc | Added new test case TEST(Variable, typeMismatch) to validate type mismatch prevention and conversion sequencing |
| test/testModel.cc | Updated test fixture with local Minsky instance and fixed a grammar error in comment |
| model/variable.h | Removed unused name parameter from ensureValueExists method declaration |
| model/variable.cc | Implemented type mismatch validation logic in ensureValueExists and removed unused parameter from all call sites |
| model/minsky.cc | Reordered operations in convertVarType to update VariableValue type before retyping variables |
| engine/equations.h | Removed unused parameter from ensureValueExists call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
This PR implements an interlock to prevent users from creating variables of different types with the same name, addressing ticket 1473.
Key Changes:
ensureValueExists()to throw an error if attempting to create/rename a variable to a name that already exists with a different typeconvertVarType()to update the VariableValue's type before retyping variables, preventing false positives during legitimate type conversionsDesign:
convertVarTypeis explicitly supported by updating the VariableValue firstThe solution is minimal, focused, and maintains backward compatibility.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
This change is
Summary by CodeRabbit
Bug Fixes
Tests