Conversation
alexcrocha
force-pushed
the
03-25-index_retroactive_method_visibility_changes
branch
from
e968172 to
a6cc2ae
Compare
alexcrocha
force-pushed
the
03-25-index_retroactive_method_visibility_changes
branch
from
a6cc2ae to
d70417d
Compare
| if args.len() != 1 { | ||
| return false; | ||
| } | ||
|
|
||
| let arg = args.iter().next().unwrap(); |
There was a problem hiding this comment.
Instead of checking the length and then unwrapping the first, we can do a single operation:
| if args.len() != 1 { | |
| return false; | |
| } | |
| let arg = args.iter().next().unwrap(); | |
| let Some(arg) = args.iter().next() else { | |
| return false; | |
| } |
There was a problem hiding this comment.
I ended up removing that inline/retroactive routing entirely. Visibility args are now handled independently, so this no longer needs the len() guard,. Mixed cases like private def foo; end, :bar now work correctly, and mixed attr_* calls like private attr_reader(:foo), :bar now get diagnosed instead of being treated as inline private
| let message = if has_any_literal { | ||
| format!("`{call_name}` called with mixed literal and non-literal arguments") | ||
| } else { | ||
| format!("`{call_name}` called with non-literal arguments") | ||
| }; |
There was a problem hiding this comment.
Instead of adding a single diagnostic for the entire visibility call, we should add one diagnostic per incorrect argument.
That makes a bit clearer to the user where the problem is.
There was a problem hiding this comment.
Fixed. With the change to visibility args being handled independently, each incorrect argument now gets its own diagnostic
|
|
||
| assert_eq!(context.graph().definitions().len(), 3); // Foo, Foo::Qux, Foo#foo | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's add some examples for changing the visibility of inherited methods. We're not going to support it immediately, but we should have it documented and maybe include a TODO comment.
There was a problem hiding this comment.
Good point. That reminded me that we already covered inherited method visibility in #691
I am leaning towards rebasing that draft and opening it for review so we can get the broader Ruby visibility behaviour coverage into the codebase, instead of cherry-picking only the cases relevant to this PR.
Does that sound like a good plan to you?
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
Ruby allows mixing inline and retroactive visibility arguments in a single call (`private def foo; end, :bar`), where both methods should become private. The previous implementation made a binary call-level decision based on the first argument, routing all args to either the inline or retroactive path and silently dropping arguments that didn't match the chosen path. We now classify and dispatch each argument independently instead of making a single call-level decision. Inline defs and retroactive literal targets can coexist in the same visibility call, and invalid arguments now emit diagnostics at their own locations instead of once for the whole call. This also preserves the existing `attr_*` special case only for the sole-argument form (`private attr_reader(:foo)`). In mixed calls like `private attr_reader(:foo), :bar`, the `attr_reader` call is no longer treated as an inline visibility target.
The same pattern for extracting a name and offset from a SymbolNode or StringNode was duplicated across each_string_or_symbol_arg, create_method_visibility_definition, and handle_constant_visibility
alexcrocha
force-pushed
the
03-25-index_retroactive_method_visibility_changes
branch
from
c3c4789 to
a11d3ce
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)