Skip to content

Commit 66bd905

Browse files
andrykonchinandrykonchin
committed
[GR-18163] Fix "unknown keyword" exception message
PullRequest: truffleruby/3419
2 parents ce5c3f0 + 7b97788 commit 66bd905

File tree

12 files changed

+223
-38
lines changed

12 files changed

+223
-38
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ Compatibility:
1919
* Fix escaping of `/` by `Regexp#source` (#2569, @andrykonchin).
2020
* Range literals of integers are now created at parse time like in CRuby (#2622, @aardvark179).
2121
* Fix `IO.pipe` - allow overriding `IO.new` that is used to create new pipes (#2692, @andykonchin).
22+
* Fix exception message when there are missing or extra keyword arguments - it contains all the missing/extra keywords now (#1522, @andrykonchin).
2223

2324
Performance:
2425

‎spec/ruby/language/keyword_arguments_spec.rb

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,24 @@ def m(*a, kw:)
5858
m(kw: 1).should == []
5959
-> { m(kw: 1, kw2: 2) }.should raise_error(ArgumentError, 'unknown keyword: :kw2')
6060
-> { m(kw: 1, true => false) }.should raise_error(ArgumentError, 'unknown keyword: true')
61+
-> { m(kw: 1, a: 1, b: 2, c: 3) }.should raise_error(ArgumentError, 'unknown keywords: :a, :b, :c')
62+
end
63+
64+
it "raises ArgumentError exception when required keyword argument is not passed" do
65+
def m(a:, b:, c:)
66+
[a, b, c]
67+
end
68+
69+
-> { m(a: 1, b: 2) }.should raise_error(ArgumentError, /missing keyword: :c/)
70+
-> { m() }.should raise_error(ArgumentError, /missing keywords: :a, :b, :c/)
71+
end
72+
73+
it "raises ArgumentError for missing keyword arguments even if there are extra ones" do
74+
def m(a:)
75+
a
76+
end
77+
78+
-> { m(b: 1) }.should raise_error(ArgumentError, /missing keyword: :a/)
6179
end
6280

6381
it "handle * and ** at the same call site" do

‎spec/ruby/language/method_spec.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,13 @@ def m(a:) a end
571571
end
572572
end
573573

574+
evaluate <<-ruby do
575+
def m(a:, **kw) [a, kw] end
576+
ruby
577+
578+
-> { m(b: 1) }.should raise_error(ArgumentError)
579+
end
580+
574581
evaluate <<-ruby do
575582
def m(a: 1) a end
576583
ruby

‎spec/ruby/language/proc_spec.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,4 +237,11 @@
237237
end
238238
end
239239
end
240+
241+
describe "taking |required keyword arguments, **kw| arguments" do
242+
it "raises ArgumentError for missing required argument" do
243+
p = proc { |a:, **kw| [a, kw] }
244+
-> { p.call() }.should raise_error(ArgumentError)
245+
end
246+
end
240247
end

‎spec/tags/language/keyword_arguments_tags.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
fails:Keyword arguments raises ArgumentError for missing keyword arguments even if there are extra ones

‎src/main/java/org/truffleruby/core/exception/CoreExceptions.java

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,18 @@ public RubyException argumentErrorCantOmitPrecision(Node currentNode) {
181181
}
182182

183183
@TruffleBoundary
184-
public RubyException argumentErrorUnknownKeyword(Object key, Node currentNode) {
185-
return argumentError("unknown keyword: " + inspect(key), currentNode);
184+
public RubyException argumentErrorUnknownKeywords(Object[] keys, Node currentNode) {
185+
if (keys.length == 1) {
186+
return argumentError("unknown keyword: " + inspect(keys[0]), currentNode);
187+
}
188+
189+
final String[] names = new String[keys.length];
190+
191+
for (int i = 0; i < keys.length; i++) {
192+
names[i] = inspect(keys[i]);
193+
}
194+
195+
return argumentError("unknown keywords: " + String.join(", ", names), currentNode);
186196
}
187197

188198
@TruffleBoundary
@@ -197,8 +207,18 @@ public RubyException argumentErrorInvalidRadix(int radix, Node currentNode) {
197207
}
198208

199209
@TruffleBoundary
200-
public RubyException argumentErrorMissingKeyword(String name, Node currentNode) {
201-
return argumentError(StringUtils.format("missing keyword: %s", name), currentNode);
210+
public RubyException argumentErrorMissingKeywords(Object[] keys, Node currentNode) {
211+
if (keys.length == 1) {
212+
return argumentError("missing keyword: " + inspect(keys[0]), currentNode);
213+
}
214+
215+
final String[] names = new String[keys.length];
216+
217+
for (int i = 0; i < keys.length; i++) {
218+
names[i] = inspect(keys[i]);
219+
}
220+
221+
return argumentError("missing keywords: " + String.join(", ", names), currentNode);
202222
}
203223

204224
@TruffleBoundary

‎src/main/java/org/truffleruby/language/arguments/CheckKeywordArityNode.java

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import com.oracle.truffle.api.CompilerDirectives;
1313
import org.truffleruby.RubyLanguage;
14+
import org.truffleruby.core.exception.RubyException;
1415
import org.truffleruby.core.hash.RubyHash;
1516
import org.truffleruby.core.hash.library.HashStoreLibrary;
1617
import org.truffleruby.core.hash.library.HashStoreLibrary.EachEntryCallback;
@@ -20,16 +21,22 @@
2021
import org.truffleruby.language.methods.Arity;
2122

2223
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
24+
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
2325
import com.oracle.truffle.api.frame.VirtualFrame;
2426
import com.oracle.truffle.api.nodes.ExplodeLoop;
2527
import com.oracle.truffle.api.profiles.BranchProfile;
2628

29+
import java.util.ArrayList;
30+
import java.util.Arrays;
31+
import java.util.List;
32+
import java.util.stream.Collectors;
33+
2734
/** Check that no extra keyword arguments are given, when there is no **kwrest */
2835
public class CheckKeywordArityNode extends RubyBaseNode {
2936

3037
public final Arity arity;
3138
@Child private ReadUserKeywordsHashNode readUserKeywordsHashNode;
32-
@Child private CheckKeywordArgumentsNode checkKeywordArgumentsNode;
39+
@Child private CheckExtraKeywordArgumentsNode checkExtraKeywordArgumentsNode;
3340

3441
public CheckKeywordArityNode(Arity arity) {
3542
assert !arity.hasKeywordsRest() : "no need to create this node";
@@ -45,37 +52,37 @@ public void checkArity(VirtualFrame frame) {
4552
}
4653

4754
private void checkKeywordArguments(RubyHash keywordArguments) {
48-
if (checkKeywordArgumentsNode == null) {
55+
if (checkExtraKeywordArgumentsNode == null) {
4956
CompilerDirectives.transferToInterpreterAndInvalidate();
50-
checkKeywordArgumentsNode = insert(new CheckKeywordArgumentsNode(getLanguage(), arity));
57+
checkExtraKeywordArgumentsNode = insert(new CheckExtraKeywordArgumentsNode(getLanguage(), arity));
5158
}
52-
checkKeywordArgumentsNode.check(keywordArguments);
59+
60+
checkExtraKeywordArgumentsNode.check(keywordArguments);
5361
}
5462

55-
private static class CheckKeywordArgumentsNode extends RubyBaseNode implements EachEntryCallback {
63+
private static class CheckExtraKeywordArgumentsNode extends RubyBaseNode implements EachEntryCallback {
5664

5765
@CompilationFinal(dimensions = 1) private final RubySymbol[] allowedKeywords;
5866

5967
private final BranchProfile unknownKeywordProfile = BranchProfile.create();
6068
@Child private HashStoreLibrary hashes;
6169

62-
public CheckKeywordArgumentsNode(RubyLanguage language, Arity arity) {
70+
public CheckExtraKeywordArgumentsNode(RubyLanguage language, Arity arity) {
6371
assert !arity.hasKeywordsRest();
6472
hashes = HashStoreLibrary.createDispatched();
6573
allowedKeywords = keywordsAsSymbols(language, arity);
6674
}
6775

6876
public void check(RubyHash keywordArguments) {
69-
hashes.eachEntry(keywordArguments.store, keywordArguments, this, null);
77+
hashes.eachEntry(keywordArguments.store, keywordArguments, this, keywordArguments);
7078
}
7179

7280
@Override
7381
public void accept(int index, Object key, Object value, Object state) {
7482
if (!keywordAllowed(key)) {
7583
unknownKeywordProfile.enter();
76-
throw new RaiseException(
77-
getContext(),
78-
coreExceptions().argumentErrorUnknownKeyword(key, this));
84+
RubyHash keywordArguments = (RubyHash) state;
85+
throw new RaiseException(getContext(), unknownKeywordsError(keywordArguments));
7986
}
8087
}
8188

@@ -89,6 +96,30 @@ private boolean keywordAllowed(Object keyword) {
8996

9097
return false;
9198
}
99+
100+
@TruffleBoundary
101+
private RubyException unknownKeywordsError(RubyHash keywordArguments) {
102+
Object[] keys = findExtraKeywordArguments(keywordArguments);
103+
return coreExceptions().argumentErrorUnknownKeywords(keys, this);
104+
}
105+
106+
@TruffleBoundary
107+
@SuppressWarnings("unchecked")
108+
private Object[] findExtraKeywordArguments(RubyHash keywordArguments) {
109+
final ArrayList<Object> actualKeywordsAsList = new ArrayList<>();
110+
hashes.eachEntry(
111+
keywordArguments.store,
112+
keywordArguments,
113+
(index, key, value, state) -> ((ArrayList<Object>) state).add(key),
114+
actualKeywordsAsList);
115+
116+
final List<RubySymbol> allowedKeywordsAsList = Arrays.asList(allowedKeywords);
117+
final List<Object> extraKeywords = actualKeywordsAsList.stream()
118+
.filter(k -> !allowedKeywordsAsList.contains(k))
119+
.collect(Collectors.toList());
120+
121+
return extraKeywords.toArray();
122+
}
92123
}
93124

94125
static RubySymbol[] keywordsAsSymbols(RubyLanguage language, Arity arity) {

‎src/main/java/org/truffleruby/language/arguments/MissingKeywordArgumentNode.java

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,22 +9,77 @@
99
*/
1010
package org.truffleruby.language.arguments;
1111

12+
import org.truffleruby.RubyLanguage;
13+
import org.truffleruby.core.exception.RubyException;
14+
import org.truffleruby.core.hash.RubyHash;
15+
import org.truffleruby.core.hash.library.HashStoreLibrary;
16+
import org.truffleruby.core.symbol.RubySymbol;
1217
import org.truffleruby.language.RubyContextSourceNode;
1318
import org.truffleruby.language.control.RaiseException;
1419

20+
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
21+
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
1522
import com.oracle.truffle.api.frame.VirtualFrame;
23+
import org.truffleruby.language.methods.Arity;
1624

17-
public class MissingKeywordArgumentNode extends RubyContextSourceNode {
25+
import java.util.ArrayList;
26+
import java.util.Arrays;
27+
import java.util.List;
28+
import java.util.stream.Collectors;
1829

19-
private final String name;
30+
public class MissingKeywordArgumentNode extends RubyContextSourceNode {
31+
@CompilationFinal(dimensions = 1) private final RubySymbol[] requiredKeywords;
32+
@Child private ReadUserKeywordsHashNode readUserKeywordsHashNode;
33+
@Child private HashStoreLibrary hashes;
2034

21-
public MissingKeywordArgumentNode(String name) {
22-
this.name = name;
35+
public MissingKeywordArgumentNode(RubyLanguage language, Arity arity) {
36+
requiredKeywords = requiredKeywordsAsSymbols(language, arity);
37+
readUserKeywordsHashNode = new ReadUserKeywordsHashNode();
38+
hashes = insert(HashStoreLibrary.createDispatched());
2339
}
2440

2541
@Override
2642
public Object execute(VirtualFrame frame) {
27-
throw new RaiseException(getContext(), coreExceptions().argumentErrorMissingKeyword(name, this));
43+
RubyHash actualKeywords = readUserKeywordsHashNode.execute(frame);
44+
throw new RaiseException(getContext(), missingKeywordsError(actualKeywords));
45+
}
46+
47+
private RubySymbol[] requiredKeywordsAsSymbols(RubyLanguage language, Arity arity) {
48+
final String[] requiredKeywords = arity.getRequiredKeywordArguments();
49+
final RubySymbol[] symbols = new RubySymbol[requiredKeywords.length];
50+
51+
for (int i = 0; i < requiredKeywords.length; i++) {
52+
symbols[i] = language.getSymbol(requiredKeywords[i]);
53+
}
54+
55+
return symbols;
2856
}
2957

58+
@TruffleBoundary
59+
private RubyException missingKeywordsError(RubyHash actualKeywords) {
60+
final Object[] missingKeywords = findMissingKeywordArguments(actualKeywords);
61+
return coreExceptions().argumentErrorMissingKeywords(missingKeywords, this);
62+
}
63+
64+
@TruffleBoundary
65+
@SuppressWarnings("unchecked")
66+
private Object[] findMissingKeywordArguments(RubyHash actualKeywords) {
67+
if (actualKeywords == null) {
68+
return requiredKeywords;
69+
}
70+
71+
final ArrayList<Object> actualKeywordsAsList = new ArrayList<>();
72+
hashes.eachEntry(
73+
actualKeywords.store,
74+
actualKeywords,
75+
(index, key, value, state) -> ((ArrayList<Object>) state).add(key),
76+
actualKeywordsAsList);
77+
78+
final List<RubySymbol> requiredKeywordsAsList = Arrays.asList(requiredKeywords);
79+
final List<RubySymbol> missingKeywords = requiredKeywordsAsList.stream()
80+
.filter(k -> !actualKeywordsAsList.contains(k))
81+
.collect(Collectors.toList());
82+
83+
return missingKeywords.toArray();
84+
}
3085
}

‎src/main/java/org/truffleruby/language/arguments/ReadKeywordArgumentNode.java

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010
package org.truffleruby.language.arguments;
1111

12+
import com.oracle.truffle.api.CompilerDirectives;
1213
import com.oracle.truffle.api.frame.Frame;
1314
import org.truffleruby.collections.PEBiFunction;
1415
import com.oracle.truffle.api.dsl.ImportStatic;
@@ -23,6 +24,8 @@
2324
import org.truffleruby.language.RubyNode;
2425

2526
import com.oracle.truffle.api.frame.VirtualFrame;
27+
import org.truffleruby.language.RubyRootNode;
28+
import org.truffleruby.language.methods.Arity;
2629

2730
/** Read a single keyword argument or execute its default value expression if missing */
2831
@ImportStatic(HashGuards.class)
@@ -52,7 +55,7 @@ protected ReadKeywordArgumentNode(RubySymbol name, RubyNode defaultValue) {
5255

5356
@Specialization(guards = "hash == null")
5457
protected Object nullHash(VirtualFrame frame, RubyHash hash) {
55-
return defaultValue.execute(frame);
58+
return getDefaultValue().execute(frame);
5659
}
5760

5861
@Specialization(guards = "hash != null", limit = "hashStrategyLimit()")
@@ -72,6 +75,20 @@ public Object accept(Frame frame, Object hash, Object key) {
7275
// This only works if the library is always cached and does not reach the limit.
7376
// Since this node is never uncached, and the limit is >= number of strategies, it should hold.
7477
final VirtualFrame virtualFrame = (VirtualFrame) frame;
75-
return defaultValue.execute(virtualFrame);
78+
return getDefaultValue().execute(virtualFrame);
79+
}
80+
81+
RubyNode getDefaultValue() {
82+
if (defaultValue == null) {
83+
// This isn't a true default value - it's a marker to say there isn't one.
84+
// This actually makes sense; the semantic action of executing this node is to report an error,
85+
// and we do the same thing.
86+
CompilerDirectives.transferToInterpreterAndInvalidate();
87+
RubyRootNode rootNode = (RubyRootNode) getRootNode();
88+
Arity arity = rootNode.getSharedMethodInfo().getArity();
89+
defaultValue = insert(new MissingKeywordArgumentNode(getLanguage(), arity));
90+
}
91+
92+
return defaultValue;
7693
}
7794
}

0 commit comments

Comments
 (0)