Disallow Linking to Parameters#757
Conversation
| } | ||
| } | ||
|
|
||
| impl<'a> TryFrom<&'a Node> for WeakPtr<dyn Entity> { |
There was a problem hiding this comment.
Now that slicec-cs is gone. This conversion function was only used by the comment_link_patcher.
So I moved this code into a simple helper function in that file.
It was only ever a convenience function anyways, letting you go from a &Node to WeakPtr that specifically points to an Entity.
| // Look up the linked-to entity in the AST. | ||
| let result = ast | ||
| .find_node_with_scope(&identifier.value, &commentable.parser_scoped_identifier()) | ||
| .and_then(<WeakPtr<dyn Entity>>::try_from); |
There was a problem hiding this comment.
Instead of using the old conversion function I deleted, we immediately handle any errors from find_node_with_scope, then call our new convert_node_to_entity_ptr helper function instead.
| Err(error) => { | ||
| let message = match error { | ||
| LookupError::DoesNotExist { identifier } => { | ||
| format!("no element named '{identifier}' exists in scope") | ||
| } | ||
| LookupError::TypeMismatch { actual, .. } => { | ||
| let type_string = match actual.as_str() { | ||
| "primitive" => "primitive types", | ||
| "module" => "modules", | ||
|
|
||
| // Only primitive types and modules have names but cannot be linked to. | ||
| _ => unreachable!(), | ||
| }; | ||
| type_string.to_owned() + " cannot be linked to" | ||
| } | ||
| }; |
There was a problem hiding this comment.
We no longer need to massage the error message from the conversion function.
Our helper function just returns the exact error message we want now.
There was a problem hiding this comment.
Pull request overview
This PR tightens Slice doc-comment link validation by disallowing @link/@see references to operation parameters (and, as implemented, any Node::Parameter), aligning compiler behavior with the intent described in #754 and prior restrictions on modules/primitive types.
Changes:
- Extend broken-link linting to reject doc links that resolve to
Node::Parameter(parameters). - Refactor comment-link patching to use a dedicated node→entity conversion helper with custom error messages.
- Add a regression test case asserting that parameter links are rejected.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
slicec/tests/comment_tests.rs |
Adds a new test case asserting @link to a parameter is rejected. |
slicec/src/patchers/comment_link_patcher.rs |
Refactors link resolution and explicitly rejects modules/primitives/parameters with clearer messages. |
slicec/src/ast/node.rs |
Removes the TryFrom<&Node> for WeakPtr<dyn Entity> conversion impl used previously for link patching. |
Comments suppressed due to low confidence (1)
slicec/src/ast/node.rs:173
Nodeis a public type (re-exported viapub mod ast), so removingimpl TryFrom<&Node> for WeakPtr<dyn Entity>is a breaking public API change for any downstream code that relied on converting AST nodes into entity pointers. If this is intentional, consider deprecating first or providing a replacement helper in the public API (or keeping the impl and making doc-link patching explicitly rejectNode::Parameter).
impl<'a> TryFrom<&'a Node> for &'a dyn Entity {
type Error = LookupError;
/// Attempts to unwrap a node to a dynamically typed reference of a Slice [Entity].
///
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR is part of #754, and disallows linking to a parameter in a doc comment with either of
@linkor@see.It also does a little adjacent cleanup now that
slicec-csis going away,Note that we already disallowed linking to modules or primitive types.