Refactor parameter validation to use specific error variants

[coderabbitai]

Context

During code review of PR #57, it was identified that the parameter parsing logic incorrectly uses ParseError::MissingColon for empty parameter names and types, which is semantically inaccurate.

Suggestion

Add specific error variants to improve error reporting clarity:

• ParseError::EmptyParameterName for parameters with empty names
• ParseError::EmptyParameterType for parameters with empty types

This would replace the current practice of using ParseError::MissingColon for these distinct validation errors.

Benefits

• More precise error reporting
• Clearer semantic meaning of error variants
• Better diagnostic messages for users

References

• Original PR: Refine parameter parsing #57
• Code review discussion: Refine parameter parsing #57 (comment)
• Addressed in commit: c15e580

Requested by: @leynos

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos I'll help you analyze whether issue #58 is still relevant by examining the codebase and documentation. Let me start by fetching the issue details and then investigate the current state of the code.

---

🏁 Script executed:

#!/bin/bash
# Fetch the GitHub issue details
gh issue view 58 --repo leynos/ddlint

Length of output: 1099

---

Now I understand the issue. It's about refactoring parameter validation to use specific error variants (ParseError::EmptyParameterName and ParseError::EmptyParameterType) instead of incorrectly using ParseError::MissingColon. The issue mentions it was addressed in commit c15e580. Let me examine the current codebase to see if this has been implemented.

---

🏁 Script executed:

#!/bin/bash
# First, let's find the ParseError enum definition
echo "=== Searching for ParseError enum ==="
ast-grep --pattern 'enum ParseError {
  $$$
}'

echo -e "\n=== Searching for ParseError variants ==="
rg "ParseError::" -A 2 -B 2

echo -e "\n=== Checking if specific variants exist ==="
rg "EmptyParameterName\|EmptyParameterType" -A 2 -B 2

Length of output: 2926

---

Great! I can see that the code is already using ParseError::MissingName and ParseError::MissingType instead of the problematic ParseError::MissingColon. The documentation also reflects this. Let me examine the ParseError enum definition and check the commit mentioned in the issue to understand what was actually implemented.

---

🏁 Script executed:

#!/bin/bash
# Let's find the ParseError enum definition
echo "=== Finding ParseError enum definition ==="
rg "enum ParseError" -A 20

echo -e "\n=== Checking the specific commit mentioned in the issue ==="
git show c15e580 --stat

echo -e "\n=== Looking at the parameter parsing logic ==="
rg "MissingName\|MissingType" -A 3 -B 3

Length of output: 2210

---

Perfect! I can see the ParseError enum has MissingName and MissingType variants instead of the originally suggested EmptyParameterName and EmptyParameterType. The commit mentioned in the issue doesn't exist in the current repository. Let me examine the actual usage of these error variants and check if the parameter parsing logic is correctly using them.

---

🏁 Script executed:

#!/bin/bash
# Let's look at the parameter parsing logic more specifically
echo "=== Searching for parameter parsing functions ==="
rg "fn.*parameter" -A 5 -B 2

echo -e "\n=== Looking at the usage of MissingName and MissingType ==="
rg -n "MissingName\|MissingType" -A 3 -B 3

echo -e "\n=== Check recent git history for changes to parse_utils.rs ==="
git log --oneline -10 -- src/parser/ast/parse_utils.rs

echo -e "\n=== Look for any remaining incorrect usage of MissingColon ==="
rg -n "MissingColon" -A 5 -B 5

Length of output: 5442

---

Now I have a clear picture! Let me examine the actual implementation of the parameter validation logic to see if it's using the correct error variants for empty names and types.

---

🏁 Script executed:

#!/bin/bash
# Let's look at the complete finalise_parameter function
echo "=== Complete finalise_parameter function ==="
ast-grep --pattern 'fn finalise_parameter($_) {
  $$$
}'

echo -e "\n=== Look for empty parameter validation logic ==="
rg -n "name.is_empty\|type.is_empty" -A 5 -B 5

echo -e "\n=== Check if MissingName and MissingType are actually used ==="
rg -n "ParseError::MissingName\|ParseError::MissingType" -A 2 -B 2

Length of output: 531

---

Let me examine the parse_utils.rs file directly to see the actual implementation of the parameter validation logic.

---

🏁 Script executed:

#!/bin/bash
# Let's read the finalise_parameter function directly
echo "=== Reading finalise_parameter function from parse_utils.rs ==="
sed -n '/fn finalise_parameter/,/^}/p' src/parser/ast/parse_utils.rs

echo -e "\n=== Look for any validation logic that checks for empty strings ==="
rg -n "\.is_empty\(\)" src/parser/ast/parse_utils.rs -A 3 -B 3

echo -e "\n=== Check the parse_name_type_pairs function ==="
sed -n '/fn parse_name_type_pairs/,/^}/p' src/parser/ast/parse_utils.rs | head -50

Length of output: 5067

---

@leynos Great question! After examining the codebase thoroughly, I can confirm that issue #58 has been resolved.

Current Implementation Status

The parameter validation logic now correctly uses specific error variants instead of the problematic ParseError::MissingColon:

✅ Issue Fixed: The code in src/parser/ast/parse_utils.rs (lines 346-357) now properly validates empty parameters:

if name.is_empty()
    && !ty.is_empty()
    && let Some(s) = span
{
    errors.push(ParseError::MissingName { span: s });
}
if !name.is_empty()
    && ty.is_empty()
    && let Some(s) = span
{
    errors.push(ParseError::MissingType { span: s });
}

✅ Correct Usage: ParseError::MissingColon is now only used for its intended purpose - when a colon is actually missing between parameter name and type (line 318).

Documentation Status

✅ Documentation Updated: The docs/function-parsing-design.md file correctly reflects the implementation:

"Empty names and types are reported with ParseError::MissingName and ParseError::MissingType"

Implementation Notes

While the original issue suggested using ParseError::EmptyParameterName and ParseError::EmptyParameterType, the implementation uses ParseError::MissingName and ParseError::MissingType - which are semantically equivalent and arguably cleaner names.

Recommendation

This issue can be closed as it has been successfully resolved. The code now provides proper semantic error reporting for empty parameter names and types, and the documentation accurately reflects the current behavior.