Skip to content

Commit 964477b

Browse files
ydahydah
committed
Add warning for rules mixing positional and named references
1 parent b5eba1e commit 964477b

4 files changed

Lines changed: 234 additions & 0 deletions

File tree

‎lib/lrama/warnings.rb‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
require_relative 'warnings/conflicts'
55
require_relative 'warnings/implicit_empty'
6+
require_relative 'warnings/mixed_references'
67
require_relative 'warnings/name_conflicts'
78
require_relative 'warnings/redefined_rules'
89
require_relative 'warnings/required'
@@ -14,6 +15,7 @@ class Warnings
1415
def initialize(logger, warnings)
1516
@conflicts = Conflicts.new(logger, warnings)
1617
@implicit_empty = ImplicitEmpty.new(logger, warnings)
18+
@mixed_references = MixedReferences.new(logger, warnings)
1719
@name_conflicts = NameConflicts.new(logger, warnings)
1820
@redefined_rules = RedefinedRules.new(logger, warnings)
1921
@required = Required.new(logger, warnings)
@@ -24,6 +26,7 @@ def initialize(logger, warnings)
2426
def warn(grammar, states)
2527
@conflicts.warn(states)
2628
@implicit_empty.warn(grammar)
29+
@mixed_references.warn(grammar)
2730
@name_conflicts.warn(grammar)
2831
@redefined_rules.warn(grammar)
2932
@required.warn(grammar)

‎lib/lrama/warnings/mixed_references.rb‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
# rbs_inline: enabled
2+
# frozen_string_literal: true
3+
4+
module Lrama
5+
class Warnings
6+
# Warning rationale: Mixing positional and named references in one rule
7+
# - It reduces readability and makes semantic actions harder to maintain
8+
# - Named references are generally more robust when RHS changes
9+
class MixedReferences
10+
# @rbs (Lrama::Logger logger, bool warnings) -> void
11+
def initialize(logger, warnings)
12+
@logger = logger
13+
@warnings = warnings
14+
end
15+
16+
# @rbs (Lrama::Grammar grammar) -> void
17+
def warn(grammar)
18+
return unless @warnings
19+
20+
build_grouped_usage(grammar).each do |rule, usage|
21+
next unless usage[:positional] && usage[:named]
22+
23+
@logger.warn("rule `#{rule.as_comment}` mixes positional and named references; use named references consistently")
24+
end
25+
end
26+
27+
private
28+
29+
# @rbs (Lrama::Grammar grammar) -> Hash[Lrama::Grammar::Rule, { positional: bool, named: bool }]
30+
def build_grouped_usage(grammar)
31+
grouped_usage = {} #: Hash[Lrama::Grammar::Rule, { positional: bool, named: bool }]
32+
33+
grammar.rules.each do |rule|
34+
next unless (token_code = rule.token_code)
35+
36+
original_rule = rule.original_rule || rule
37+
usage = (grouped_usage[original_rule] ||= { positional: false, named: false })
38+
classify_references(token_code.references, usage)
39+
end
40+
41+
grouped_usage
42+
end
43+
44+
# @rbs (Array[Lrama::Grammar::Reference] references, { positional: bool, named: bool } usage) -> void
45+
def classify_references(references, usage)
46+
references.each do |ref|
47+
if positional_reference?(ref)
48+
usage[:positional] = true
49+
elsif named_reference?(ref)
50+
usage[:named] = true
51+
end
52+
end
53+
end
54+
55+
# @rbs (Lrama::Grammar::Reference ref) -> bool
56+
def positional_reference?(ref)
57+
return false if ref.index.nil?
58+
59+
ref.name.nil?
60+
end
61+
62+
# @rbs (Lrama::Grammar::Reference ref) -> bool
63+
def named_reference?(ref)
64+
# Ignore special references like $$, @$ and $:$.
65+
!ref.name.nil? && ref.name != "$"
66+
end
67+
end
68+
end
69+
end

‎sig/generated/lrama/warnings/mixed_references.rbs‎

Lines changed: 30 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎spec/lrama/warnings/mixed_references_spec.rb‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe Lrama::Warnings::MixedReferences do
4+
describe "#warn" do
5+
let(:mixed_references_y) do
6+
<<~STR
7+
%{
8+
// Prologue
9+
%}
10+
%union {
11+
int i;
12+
}
13+
%token <i> NUM
14+
%type <i> expr
15+
%%
16+
program: expr
17+
;
18+
expr[result]: NUM[left] NUM[right]
19+
{
20+
$result = $1 + $right;
21+
}
22+
;
23+
STR
24+
end
25+
26+
let(:named_references_only_y) do
27+
<<~STR
28+
%{
29+
// Prologue
30+
%}
31+
%union {
32+
int i;
33+
}
34+
%token <i> NUM
35+
%type <i> expr
36+
%%
37+
program: expr
38+
;
39+
expr[result]: NUM[left] NUM[right]
40+
{
41+
$result = $left + $right;
42+
}
43+
;
44+
STR
45+
end
46+
47+
let(:mixed_across_actions_y) do
48+
<<~STR
49+
%{
50+
// Prologue
51+
%}
52+
%union {
53+
int i;
54+
}
55+
%token <i> NUM
56+
%type <i> expr
57+
%%
58+
program: expr
59+
;
60+
expr[result]: NUM
61+
{
62+
$1;
63+
}
64+
NUM[right]
65+
{
66+
$result = $right;
67+
}
68+
;
69+
STR
70+
end
71+
72+
context "when warnings true" do
73+
it "warns about mixed positional and named references in a rule" do
74+
grammar = Lrama::Parser.new(mixed_references_y, "warnings/mixed_references.y").parse
75+
grammar.prepare
76+
grammar.validate!
77+
states = Lrama::States.new(grammar, Lrama::Tracer.new(Lrama::Logger.new))
78+
states.compute
79+
logger = Lrama::Logger.new
80+
allow(logger).to receive(:warn)
81+
82+
Lrama::Warnings.new(logger, true).warn(grammar, states)
83+
84+
expect(logger).to have_received(:warn).with("rule `expr: NUM NUM` mixes positional and named references; use named references consistently")
85+
end
86+
87+
it "does not warn when named references are used consistently" do
88+
grammar = Lrama::Parser.new(named_references_only_y, "warnings/named_references_only.y").parse
89+
grammar.prepare
90+
grammar.validate!
91+
states = Lrama::States.new(grammar, Lrama::Tracer.new(Lrama::Logger.new))
92+
states.compute
93+
logger = Lrama::Logger.new
94+
allow(logger).to receive(:warn)
95+
96+
Lrama::Warnings.new(logger, true).warn(grammar, states)
97+
98+
expect(logger).not_to have_received(:warn).with(/mixes positional and named references/)
99+
end
100+
101+
it "warns once when references are mixed across actions in the same rule" do
102+
grammar = Lrama::Parser.new(mixed_across_actions_y, "warnings/mixed_across_actions.y").parse
103+
grammar.prepare
104+
grammar.validate!
105+
states = Lrama::States.new(grammar, Lrama::Tracer.new(Lrama::Logger.new))
106+
states.compute
107+
logger = Lrama::Logger.new
108+
allow(logger).to receive(:warn)
109+
110+
Lrama::Warnings.new(logger, true).warn(grammar, states)
111+
112+
expect(logger).to have_received(:warn).with(/rule `expr: .*` mixes positional and named references; use named references consistently/).once
113+
end
114+
end
115+
116+
context "when warnings false" do
117+
it "does not warn even if references are mixed" do
118+
grammar = Lrama::Parser.new(mixed_references_y, "warnings/mixed_references.y").parse
119+
grammar.prepare
120+
grammar.validate!
121+
states = Lrama::States.new(grammar, Lrama::Tracer.new(Lrama::Logger.new))
122+
states.compute
123+
logger = Lrama::Logger.new
124+
allow(logger).to receive(:warn)
125+
126+
Lrama::Warnings.new(logger, false).warn(grammar, states)
127+
128+
expect(logger).not_to have_received(:warn)
129+
end
130+
end
131+
end
132+
end

0 commit comments

Comments
 (0)