Conversation
alexcrocha
force-pushed
the
03-25-index_retroactive_method_visibility_changes
branch
from
e968172 to
a6cc2ae
Compare
| }); | ||
|
|
||
| let is_scoped_definition = | ||
| message.as_str() == "module_function" || single_arg_is_def || single_arg_is_attr_helper; |
There was a problem hiding this comment.
It seems that module_function works like the other cases. It accepts retroactive definitions and multiple names. Could we not handle it the same? Does it require any different logic?
There was a problem hiding this comment.
I added a new commit to handle retroactive module_function as well. It goes through the same logic with a extra guard to emit a diagnostic when used outside a module
| let has_single_arg = args.len() == 1; | ||
|
|
||
| let single_arg_is_def = | ||
| has_single_arg && matches!(args.iter().next().unwrap(), ruby_prism::Node::DefNode { .. }); | ||
|
|
||
| let single_arg_is_attr_helper = has_single_arg | ||
| && args.iter().next().unwrap().as_call_node().is_some_and(|call| { | ||
| let has_no_explicit_receiver = call.receiver().is_none() | ||
| || call.receiver().as_ref().is_some_and(|r| r.as_self_node().is_some()); | ||
| has_no_explicit_receiver | ||
| && matches!( | ||
| call.name().as_slice(), | ||
| b"attr" | b"attr_reader" | b"attr_writer" | b"attr_accessor" | ||
| ) | ||
| }); |
There was a problem hiding this comment.
There are a lot of checks in this section. Would it make it a bit easier to read if we extracted a helper method?
There was a problem hiding this comment.
You'd be scared to see how it looked like before 😱 But I agree, there's still room for improvement
I added a new commit, let me know if you find it better
| } | ||
|
|
||
| // Nested references still indexed | ||
| assert_constant_references_eq!(&context, ["SOME_CONST"]); |
There was a problem hiding this comment.
Since I folded the old index_retroactive_method_visibility_dynamic_visits_args test into this one as suggested below, I'll keep this and update the comment to make the intent more explicit that we are still indexing the constant reference despite the diagnostic
| } | ||
|
|
||
| #[test] | ||
| fn index_retroactive_method_visibility() { |
There was a problem hiding this comment.
Can we create a dedicated test mod for these tests?
There was a problem hiding this comment.
#672 is introducing a visibility tests mod. Depending on which one merges first I'll update the other
Index local 'private/protected/public :foo' calls as MethodVisibility definitions so rubydex preserves retroactive method visibility changes in the graph using the same definition based model as const visibility
alexcrocha
force-pushed
the
03-25-index_retroactive_method_visibility_changes
branch
from
a6cc2ae to
d70417d
Compare
3 participants
One more step towards #89
This PR extends the definition-based visibility path from constants to methods by indexing retroactive
private/protected/publiccalls asMethodVisibilityDefinitions.Why
Ruby supports retroactive visibility calls like
private :foo, but the current codebase only models visibility from flag mode (privatefollowed by later definitions) and scoped definitions (private def fooorprotected attr_reader :bar). We already use a definition-based approach for constant visibility, so this applies the same model to methods and preserves those visibility changes in the graph.This PR is intentionally limited to indexing. A follow-up PR will make
MethodVisibilityDefinitions affect the resolved declaration graph.What
Definition::MethodVisibilityandMethodVisibilityDefinitionprivate/protected/publiccalls with literal symbol and string targetsfoo()private)private def foo)protected attr_reader :bar)