Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
…21089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573)

Changelog
for (Master)[bazelbuild/bazel@be96ade#diff-37cd73cae488b2421089c2b94c9a869cfb4a5f109753f60602b1dd20959bc573]:

- Builtins injection: Rename _internal to _builtins and add functionality  …  This object is used in @_builtins .bzl files but is not accessible to user code. The `toplevel` and `native` fields give access to the *native* (pre-injected) values of symbols whose post-injected values are available to user .bzl files. For instance, -`_builtins.toplevel.CcInfo` in @_builtins code gives the original CcInfo definition from the Java code, even if `CcInfo` in a regular .bzl file refers to an injected value. (To avoid ambiguity, `CcInfo` itself is not a valid top-level symbol for @_builtins .bzl files.) The `internal` field contains any value registered via ConfiguredRuleClassProvider.Builder#addStarlarkBuiltinsInternal(). The `getFlag()` method can retrieve the values of StarlarkSemantics flags. Because of how flags are stored, it requires that a default value be given.
-  Starlark: better errors on integer overflow  …  Before:
```
>> [1] * (1 << 30) * (1 << 5)
Exception in thread "main" net.starlark.java.eval.Starlark$UncheckedEvalException: java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1073741824 out of bounds for object array[0] (Starlark stack: [<toplevel>@<stdin>:1:1])
net.starlark.java.eval.Starlark.fastcall(Starlark.java:621)
net.starlark.java.eval.Starlark.execFileProgram(Starlark.java:892)
...
exit 1
```

Now:
```
>> [1] * (1 << 30) * (1 << 5) Traceback (most recent call last):
File "<stdin>", line 1, column 21, in <toplevel>
Error: got 34359738368 for repeat, want value in signed 32-bit range
```
-  starlark: int: ignore base prefix unless matches explicit base  …  Previously, when int was called with an explicit base, it would report an error if the digit string starts with a base prefix for a different base, such as int("0b101", 16). Now, it uses the base prefix only if it matches the requested base, so the example above would return 0x0b101, as would int("0x0b101", 16). See github.com/google/starlark-go/pull/344
-  starlark: delete StarlarkFile.parseWithPrelude  …  It was an obstacle to interpreter optimizations, as it caused a single StarlarkFile to contain statements whose locations come from different files. The previous workspace logic used parseWithPrelude to concatenate all the statements of the WORKSPACE files (prefix + main + suffix) and then split them into chunks, ignoring---crucially, several days work revealed---file boundaries. The splitChunks logic now achieves the same effect by representing chunks as lists of nonempty partial files, and calling execute() for each partial file in the chunk. The chunk splitting tests have been rewritten, clarified, and expanded to exercise the chunk-spans-file-boundaries case. Many thanks to Jon Brandvein for hours of help trying to figure out what the workspace logic does. It is not our team's finest work. Also: - minor consequent simplifications to parser and lexer. - narrow the scope of various try blocks (sorry, messy diff).
-  starlark: remove redundant pattern validity check in Printer  …
-  starlark: support %x, %X, and %o conversions in 'string % number'  …  Also, improve operand type error message to show type, not value. See github.com/bazelbuild/starlark/pull/154 for spec change.
-  Starlark: StarlarkInt.{floordiv,mod} now work with 64-bit integers  …  * No special case for 32-bit integers, division is slow anyway * Switch `mod` to use `Math.floorMod` for simplicity/safety Closes #12667.
-  starlark: delete deprecated EvalException(Location) constructor  …  ...as there are, at long last, no further uses within Bazel. Also, clarify doc comments regarding EvalException.callstack.
-  bazel analysis: preparatory cleanup to SRCTU error reporting  …  This change causes the error handling logic in StarlarkRuleConfiguredTargetUtil (SRCTU) to avoid the EvalException(Location) constructor. Instead, the auxiliary location, if any, of provider instantiation is simply prepended to the error message (see the infoError function) in anticipation of being printed after a newline. For example:  ERROR p/BUILD:1:1:\n  foo.bzl:1:2: <provider-related message> createTarget uses a distinct exception, BadImplementationFunction, for errors that arise not from the Starlark implementation function but from the post-processing of the providers. A follow-up change will replace this exception by directly reporting events to the handler, thus permitting both higher quality structured errors capable of reporting more than one relevant location, and multiple errors in a single pass. However, that change is too tricky to accomplish in this CL. Also: - move "advertised provider" and "orphaned file" checks into createTarget. - add comments to document this particularly painful code. - delete EvalException.getDeprecatedLocation.
-  Starlark: long StarlarkInt multiply without BigInteger  …  Use Hacker's Delight 8-2. Closes #12643.
-  bazel packages: use EventHandler not EvalException in .bzl "export" o…  …  …peration This allows us to delete one of the deprecated EvalException(Location,...) constructors. Similar follow-up changes (events not exceptions) will be required to eliminate the remaining such constructor. As a bonus, this approach allows multiple errors to be reported at once.
  • Loading branch information
mahmoudimus committed Feb 9, 2021
1 parent 44f9c75 commit 9d4ac63
Show file tree
Hide file tree
Showing 21 changed files with 536 additions and 531 deletions.
2 changes: 1 addition & 1 deletion .tmp/bazel
Submodule bazel updated 1903 files
59 changes: 33 additions & 26 deletions libstarlark/src/main/java/net/starlark/java/eval/Printer.java
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ public static void format(Printer printer, String format, Object... arguments) {
}

/** Same as {@link #format}, but with a list instead of variadic args. */
@SuppressWarnings("FormatString") // see b/178189609
public static void formatWithList(Printer printer, String pattern, List<?> arguments) {
// N.B. MissingFormatWidthException is the only kind of IllegalFormatException
// whose constructor can take and display arbitrary error message, hence its use below.
Expand Down Expand Up @@ -330,16 +331,6 @@ public static void formatWithList(Printer printer, String pattern, List<?> argum
continue;
}

// valid?
if ("drsefgEFG".indexOf(conv) < 0) {
throw new MissingFormatWidthException(
// The call to Starlark.repr doesn't cause an infinite recursion because it's
// only used to format a string properly.
String.format(
"unsupported format character \"%s\" at index %s in %s",
String.valueOf(conv), p + 1, Starlark.repr(pattern)));
}

// get argument
if (a >= argLength) {
throw new MissingFormatWidthException(
Expand All @@ -352,22 +343,32 @@ public static void formatWithList(Printer printer, String pattern, List<?> argum

switch (conv) {
case 'd':
if (arg instanceof StarlarkInt || arg instanceof Integer) {
printer.repr(arg);
} else if (arg instanceof StarlarkFloat) {
double d = ((StarlarkFloat) arg).toDouble();
StarlarkInt rounded;
try {
rounded = StarlarkInt.ofFiniteDouble(d);
} catch (IllegalArgumentException unused) {
throw new MissingFormatWidthException("got " + arg + ", want a finite number");
case 'o':
case 'x':
case 'X':
{
Number n;
if (arg instanceof StarlarkInt) {
n = ((StarlarkInt) arg).toNumber();
} else if (arg instanceof Integer) {
n = (Number) arg;
} else if (arg instanceof StarlarkFloat) {
double d = ((StarlarkFloat) arg).toDouble();
try {
n = StarlarkInt.ofFiniteDouble(d).toNumber();
} catch (IllegalArgumentException unused) {
throw new MissingFormatWidthException("got " + arg + ", want a finite number");
}
} else {
throw new MissingFormatWidthException(
String.format(
"got %s for '%%%c' format, want int or float", Starlark.type(arg), conv));
}
printer.repr(rounded);
} else {
throw new MissingFormatWidthException(
"invalid argument " + Starlark.repr(arg) + " for format pattern %d");
printer.str(
String.format(
conv == 'd' ? "%d" : conv == 'o' ? "%o" : conv == 'x' ? "%x" : "%X", n));
continue;
}
continue;

case 'e':
case 'f':
Expand All @@ -384,7 +385,8 @@ public static void formatWithList(Printer printer, String pattern, List<?> argum
v = ((StarlarkFloat) arg).toDouble();
} else {
throw new MissingFormatWidthException(
"invalid argument " + Starlark.repr(arg) + " for format pattern %d");
String.format(
"got %s for '%%%c' format, want int or float", Starlark.type(arg), conv));
}
printer.str(StarlarkFloat.format(v, conv));
continue;
Expand All @@ -398,7 +400,12 @@ public static void formatWithList(Printer printer, String pattern, List<?> argum
continue;

default:
throw new IllegalStateException("unreachable");
// The call to Starlark.repr doesn't cause an infinite recursion
// because it's only used to format a string properly.
throw new MissingFormatWidthException(
String.format(
"unsupported format character \"%s\" at index %s in %s",
String.valueOf(conv), p + 1, Starlark.repr(pattern)));
}
}
if (a < argLength) {
Expand Down
40 changes: 20 additions & 20 deletions libstarlark/src/main/java/net/starlark/java/eval/StarlarkInt.java
Original file line number Diff line number Diff line change
Expand Up @@ -121,14 +121,9 @@ public static StarlarkInt parse(String s, int base) {
prefixBase = 16;
}
if (prefixBase != 0) {
digits = s.substring(2); // strip prefix
if (base == 0) {
if (base == 0 || base == prefixBase) {
base = prefixBase;
} else if (base != prefixBase) {
throw new NumberFormatException(
String.format(
"invalid base-%d literal: %s (%s prefix wants base %d)",
base, Starlark.repr(stringForErrors), s.substring(0, 2), prefixBase));
digits = s.substring(2); // strip prefix
}
}
}
Expand Down Expand Up @@ -577,12 +572,19 @@ public static StarlarkInt floordiv(StarlarkInt x, StarlarkInt y) throws EvalExce
if (y == ZERO) {
throw Starlark.errorf("integer division by zero");
}
if (x instanceof Int32 && y instanceof Int32) {
long xl = ((Int32) x).v;
long yl = ((Int32) y).v;
try {
long xl = x.toLongFast();
long yl = y.toLongFast();
// http://python-history.blogspot.com/2010/08/why-pythons-integer-division-floors.html
long quo = Math.floorDiv(xl, yl);
return StarlarkInt.of(quo);
if (xl == Long.MIN_VALUE && yl == -1) {
/* sole case in which quotient doesn't fit in long */
} else {
long quo = Math.floorDiv(xl, yl);
return StarlarkInt.of(quo);
}
/* overflow */
} catch (Overflow unused) {
/* fall through */
}

BigInteger xbig = x.toBigInteger();
Expand All @@ -599,15 +601,13 @@ public static StarlarkInt mod(StarlarkInt x, StarlarkInt y) throws EvalException
if (y == ZERO) {
throw Starlark.errorf("integer modulo by zero");
}
if (x instanceof Int32 && y instanceof Int32) {
long xl = ((Int32) x).v;
long yl = ((Int32) y).v;
try {
long xl = x.toLongFast();
long yl = y.toLongFast();
// In Starlark, the sign of the result is the sign of the divisor.
long z = xl % yl;
if ((xl < 0) != (yl < 0) && z != 0) {
z += yl;
}
return StarlarkInt.of(z);
return StarlarkInt.of(Math.floorMod(xl, yl));
} catch (Overflow unused) {
/* fall through */
}

BigInteger xbig = x.toBigInteger();
Expand Down
30 changes: 23 additions & 7 deletions libstarlark/src/main/java/net/starlark/java/eval/StarlarkList.java
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@
public final class StarlarkList<E> extends AbstractList<E>
implements Sequence<E>, StarlarkValue, Mutability.Freezable, Comparable<StarlarkList<?>> {

// It's always possible to overeat in small bites but we'll
// try to stop someone swallowing the world in one gulp.
static final int MAX_ALLOC = 1 << 30;

// The implementation strategy is similar to ArrayList,
// but without the extra indirection of using ArrayList.

Expand Down Expand Up @@ -284,9 +288,12 @@ public StarlarkList<E> repeat(StarlarkInt n, Mutability mutability) throws EvalE
return wrap(mutability, EMPTY_ARRAY);
}

// TODO(adonovan): reject unreasonably large n.
int ni = n.toInt("repeat");
Object[] res = new Object[ni * size];
long sz = (long) ni * size;
if (sz > MAX_ALLOC) {
throw Starlark.errorf("excessive repeat (%d * %d elements)", size, ni);
}
Object[] res = new Object[(int) sz];
for (int i = 0; i < ni; i++) {
System.arraycopy(elems, 0, res, i * size, size);
}
Expand Down Expand Up @@ -334,14 +341,23 @@ private void grow(int mincap) {
}
}

// Grow capacity enough to insert given number of elements
private void growAdditional(int additional) throws EvalException {
int mincap = size + additional;
if (mincap < 0 || mincap > MAX_ALLOC) {
throw Starlark.errorf("excessive capacity requested (%d + %d elements)", size, additional);
}
grow(mincap);
}

/**
* Appends an element to the end of the list, after validating that mutation is allowed.
*
* @param element the element to add
*/
public void addElement(E element) throws EvalException {
Starlark.checkMutable(this);
grow(size + 1);
growAdditional(1);
elems[size++] = element;
}

Expand All @@ -353,7 +369,7 @@ public void addElement(E element) throws EvalException {
*/
public void addElementAt(int index, E element) throws EvalException {
Starlark.checkMutable(this);
grow(size + 1);
growAdditional(1);
System.arraycopy(elems, index, elems, index + 1, size - index);
elems[index] = element;
size++;
Expand All @@ -369,20 +385,20 @@ public void addElements(Iterable<? extends E> elements) throws EvalException {
if (elements instanceof StarlarkList) {
StarlarkList<?> that = (StarlarkList) elements;
// (safe even if this == that)
grow(this.size + that.size);
growAdditional(that.size);
System.arraycopy(that.elems, 0, this.elems, this.size, that.size);
this.size += that.size;
} else if (elements instanceof Collection) {
// collection of known size
Collection<?> that = (Collection) elements;
grow(size + that.size());
growAdditional(that.size());
for (Object x : that) {
elems[size++] = x;
}
} else {
// iterable
for (Object x : elements) {
grow(size + 1);
growAdditional(1);
elems[size++] = x;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,25 @@ public <T> T get(Key<T> key) {
return v != null ? v : key.defaultValue;
}

// TODO(bazel-team): This exists solely for BuiltinsInternalModule#getFlag, which allows a
// (privileged) Starlark caller to programmatically retrieve a flag's value without knowing its
// schema and default value. Reconsider whether we should support that use case from this class.
/**
* Returns the value of the option with the given name, or the default value if it is not set or
* does not exist.
*/
public Object getGeneric(String name, Object defaultValue) {
Object v = map.get(name);
// Try boolean prefixes if that didn't work.
if (v == null) {
v = map.get("+" + name);
}
if (v == null) {
v = map.get("-" + name);
}
return v != null ? v : defaultValue;
}

/** A Key identifies an option, providing its name, type, and default value. */
public static class Key<T> {
public final String name;
Expand Down
7 changes: 5 additions & 2 deletions libstarlark/src/main/java/net/starlark/java/eval/Tuple.java
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,12 @@ Tuple repeat(StarlarkInt n) throws EvalException {
return empty();
}

// TODO(adonovan): reject unreasonably large n.
int ni = n.toInt("repeat");
Object[] res = new Object[ni * elems.length];
long sz = (long) ni * elems.length;
if (sz > StarlarkList.MAX_ALLOC) {
throw Starlark.errorf("excessive repeat (%d * %d elements)", elems.length, ni);
}
Object[] res = new Object[(int) sz];
for (int i = 0; i < ni; i++) {
System.arraycopy(elems, 0, res, i * elems.length, elems.length);
}
Expand Down
9 changes: 4 additions & 5 deletions libstarlark/src/main/java/net/starlark/java/syntax/Lexer.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
package net.starlark.java.syntax;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -53,7 +53,7 @@ final class Lexer {
// The first (outermost) element is always zero.
private final Stack<Integer> indentStack = new Stack<>();

private final List<Comment> comments;
private final ImmutableList.Builder<Comment> comments = ImmutableList.builder();

// The number of unclosed open-parens ("(", '{', '[') at the current point in
// the stream. Whitespace is handled differently when this is nonzero.
Expand Down Expand Up @@ -93,14 +93,13 @@ final class Lexer {
this.pos = 0;
this.errors = errors;
this.checkIndentation = true;
this.comments = new ArrayList<>();
this.dents = 0;

indentStack.push(0);
}

List<Comment> getComments() {
return comments;
ImmutableList<Comment> getComments() {
return comments.build();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ static final class ParseResult {
final ImmutableList<Statement> statements;

/** The comments from the parsed file. */
final List<Comment> comments;
final ImmutableList<Comment> comments;

// Errors encountered during scanning or parsing.
// These lists are ultimately owned by StarlarkFile.
Expand All @@ -47,7 +47,7 @@ static final class ParseResult {
private ParseResult(
FileLocations locs,
ImmutableList<Statement> statements,
List<Comment> comments,
ImmutableList<Comment> comments,
List<SyntaxError> errors) {
this.locs = locs;
// No need to copy here; when the object is created, the parser instance is just about to go
Expand Down
Loading

0 comments on commit 9d4ac63

Please sign in to comment.