Skip to content

Commit

Permalink
Suggest alternative table keys on nil errors (#2097)
Browse files Browse the repository at this point in the history 
We now suggest alternative table keys when code errors with "attempt
to index/call 'foo' (a nil value)". For example: "redstone.getinput()",
will now suggest "Did you mean: getInput".

This is a bit tricky to get right! In the above example, our code reads
like:

   1    GETTABUP 0 0 0 ; r0 := _ENV["redstone"]
   2    GETFIELD 0 0 1 ; r0 := r0["getinput"]
   3    CALL 0 1 1     ; r0()

Note, that when we get to the problematic line, we don't have access to
the original table that we attempted to index. In order to do this, we
borrow ideas from Lua's getobjname — we effectively write an evaluator
that walks back over the code and tries to reconstruct the expression
that resulted in nil.

For example, in the above case:
 - We know an instruction happened at pc=3, so we try to find the
   expression that computed r0.
 - We know this was set at pc=2, so we step back one. This is a GETFIELD
   instruction, so we check the key (it's a constant, so worth
   reporting), and then try to evaluate the table.
 - This version of r0 was set at pc=1, so we step back again. It's a
   GETTABUP instruction, so we can just evaluate that directly.

We then use this information (indexing _ENV.redstone with "getinput") to
find alternative keys (e.g. getInput, getOutput, etc...) and then pick
some likely suggestions with Damerau-Levenshtein/OSD.

I'm not entirely thrilled by the implementation here. The core
interpretation logic is implemented in Java. Which is *fine*, but a)
feels a little cheaty and b) means we're limited to what Lua bytecode
can provide (for instance, we can't inspect outer functions, or list all
available names in scope). We obviously can expand the bytecode if
needed, but something we'd want to be careful with.

The alternative approach would be to handle all the parsing in
Lua. Unfortunately, this is quite hard to get right — I think we'd need
some lazy parsing strategy to avoid constructing the whole AST, while
still retaining all the scope information we need.

I don't know. We really could make this as complex as we like, and I
don't know what the right balance is. It'd be cool to detect patterns
like the following, but is it *useful*?

    local monitor = peripheral.wrap("left")
    monitor.write("Hello")
        -- ^ monitor is nil. Is there a peripheral to the left of the
        -- computer?

For now, the current approach feels the easiest, and should allow us to
prototype things and see what does/doesn't work.
  • Loading branch information
@SquidDev
SquidDev authored Feb 13, 2025
1 parent 051c70a commit b185d08
Show file tree
Hide file tree
Showing 8 changed files with 573 additions and 4 deletions.
2 changes: 2 additions & 0 deletions projects/core/src/main/java/dan200/computercraft/core/lua/CobaltLuaMachine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import dan200.computercraft.core.CoreConfig;
import dan200.computercraft.core.Logging;
import dan200.computercraft.core.computer.TimeoutState;
import dan200.computercraft.core.lua.errorinfo.ErrorInfoLib;
import dan200.computercraft.core.methods.LuaMethod;
import dan200.computercraft.core.methods.MethodSupplier;
import dan200.computercraft.core.util.LuaUtil;
Expand Down Expand Up @@ -77,6 +78,7 @@ public CobaltLuaMachine(MachineEnvironment environment, InputStream bios) throws
var globals = state.globals();
CoreLibraries.debugGlobals(state);
Bit32Lib.add(state, globals);
ErrorInfoLib.add(state);
globals.rawset("_HOST", ValueFactory.valueOf(environment.hostString()));
globals.rawset("_CC_DEFAULT_SETTINGS", ValueFactory.valueOf(CoreConfig.defaultComputerSettings));

Expand Down
64 changes: 64 additions & 0 deletions projects/core/src/main/java/dan200/computercraft/core/lua/errorinfo/DebugHelpers.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// SPDX-FileCopyrightText: 2009-2011 Luaj.org, 2015-2020 SquidDev
//
// SPDX-License-Identifier: MIT

package dan200.computercraft.core.lua.errorinfo;

import org.squiddev.cobalt.Prototype;

import static org.squiddev.cobalt.Lua.*;

/**
* Extracted parts of Cobalt's {@link org.squiddev.cobalt.debug.DebugHelpers}.
*/
final class DebugHelpers {
private DebugHelpers() {
}

private static int filterPc(int pc, int jumpTarget) {
return pc < jumpTarget ? -1 : pc;
}

/**
* Find the PC where a register was last set.
* <p>
* This makes some assumptions about the structure of the bytecode, namely that there are no back edges within the
* CFG. As a result, this is only valid for temporary values, and not locals.
*
* @param pt The function prototype.
* @param lastPc The PC to work back from.
* @param reg The register.
* @return The last instruction where the register was set, or {@code -1} if not defined.
*/
static int findSetReg(Prototype pt, int lastPc, int reg) {
var lastInsn = -1; // Last instruction that changed "reg";
var jumpTarget = 0; // Any code before this address is conditional

for (var pc = 0; pc < lastPc; pc++) {
var i = pt.code[pc];
var op = GET_OPCODE(i);
var a = GETARG_A(i);
switch (op) {
case OP_LOADNIL -> {
var b = GETARG_B(i);
if (a <= reg && reg <= a + b) lastInsn = filterPc(pc, jumpTarget);
}
case OP_TFORCALL -> {
if (a >= a + 2) lastInsn = filterPc(pc, jumpTarget);
}
case OP_CALL, OP_TAILCALL -> {
if (reg >= a) lastInsn = filterPc(pc, jumpTarget);
}
case OP_JMP -> {
var dest = pc + 1 + GETARG_sBx(i);
// If jump is forward and doesn't skip lastPc, update jump target
if (pc < dest && dest <= lastPc && dest > jumpTarget) jumpTarget = dest;
}
default -> {
if (testAMode(op) && reg == a) lastInsn = filterPc(pc, jumpTarget);
}
}
}
return lastInsn;
}
}
222 changes: 222 additions & 0 deletions projects/core/src/main/java/dan200/computercraft/core/lua/errorinfo/ErrorInfoLib.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
// SPDX-FileCopyrightText: 2025 The CC: Tweaked Developers
//
// SPDX-License-Identifier: MPL-2.0

package dan200.computercraft.core.lua.errorinfo;

import com.google.common.annotations.VisibleForTesting;
import org.squiddev.cobalt.*;
import org.squiddev.cobalt.debug.DebugFrame;
import org.squiddev.cobalt.function.LuaFunction;
import org.squiddev.cobalt.function.RegisteredFunction;

import javax.annotation.Nullable;
import java.util.Objects;

import static org.squiddev.cobalt.Lua.*;
import static org.squiddev.cobalt.debug.DebugFrame.FLAG_ANY_HOOK;

/**
* Provides additional info about an error.
* <p>
* This is currently an internal and deeply unstable module. It's not clear if doing this via bytecode (rather than an
* AST) is the correct approach and/or, what the correct design is.
*/
public class ErrorInfoLib {
private static final int MAX_DEPTH = 8;

private static final RegisteredFunction[] functions = new RegisteredFunction[]{
RegisteredFunction.ofV("info_for_nil", ErrorInfoLib::getInfoForNil),
};

public static void add(LuaState state) throws LuaError {
state.registry().getSubTable(Constants.LOADED).rawset("cc.internal.error_info", RegisteredFunction.bind(functions));
}

private static Varargs getInfoForNil(LuaState state, Varargs args) throws LuaError {
var thread = args.arg(1).checkThread();
var level = args.arg(2).checkInteger();

var context = getInfoForNil(state, thread, level);
return context == null ? Constants.NIL : ValueFactory.varargsOf(
ValueFactory.valueOf(context.op()), ValueFactory.valueOf(context.source().isGlobal()),
context.source().table(), context.source().key()
);
}

/**
* Get some additional information about an {@code attempt to $OP (a nil value)} error. This often occurs as a
* result of a misspelled local, global or table index, and so we attempt to detect those cases.
*
* @param state The current Lua state.
* @param thread The thread which has errored.
* @param level The level where the error occurred. We currently expect this to always be 0.
* @return Some additional information about the error, where available.
*/
@VisibleForTesting
static @Nullable NilInfo getInfoForNil(LuaState state, LuaThread thread, int level) {
var frame = thread.getDebugState().getFrame(level);
if (frame == null || frame.closure == null || (frame.flags & FLAG_ANY_HOOK) != 0) return null;

var prototype = frame.closure.getPrototype();
var pc = frame.pc;
var insn = prototype.code[pc];

// Find what operation we're doing that errored.
return switch (GET_OPCODE(insn)) {
case OP_CALL, OP_TAILCALL ->
NilInfo.of("call", resolveValueSource(state, frame, prototype, pc, GETARG_A(insn), 0));
case OP_GETTABLE, OP_SETTABLE, OP_SELF ->
NilInfo.of("index", resolveValueSource(state, frame, prototype, pc, GETARG_A(insn), 0));
default -> null;
};
}

/**
* Information about an {@code attempt to $OP (a nil value)} error.
*
* @param op The operation we tried to perform.
* @param source The expression that resulted in a nil value.
*/
@VisibleForTesting
record NilInfo(String op, ValueSource source) {
public static @Nullable NilInfo of(String op, @Nullable ValueSource values) {
return values == null ? null : new NilInfo(op, values);
}
}

/**
* A partially-reconstructed Lua expression. This currently only is used for table indexing ({@code table[key]}.
*
* @param isGlobal Whether this is a global table access. This is a best-effort guess, and does not distinguish between
* {@code foo} and {@code _ENV.foo}.
* @param table The table being indexed.
* @param key The key we tried to index.
*/
@VisibleForTesting
record ValueSource(boolean isGlobal, LuaValue table, LuaString key) {
}

/**
* Attempt to partially reconstruct a Lua expression from the current debug state.
*
* @param state The current Lua state.
* @param frame The current debug frame.
* @param prototype The current function.
* @param pc The current program counter.
* @param register The register where this value was stored.
* @param depth The current depth. Starts at 0, and aborts once reaching {@link #MAX_DEPTH}.
* @return The reconstructed expression, or {@code null} if not available.
*/
@SuppressWarnings("NullTernary")
private static @Nullable ValueSource resolveValueSource(LuaState state, DebugFrame frame, Prototype prototype, int pc, int register, int depth) {
if (depth > MAX_DEPTH) return null;
if (prototype.getLocalName(register + 1, pc) != null) return null;

// Find where this register was set. If unknown, then abort.
pc = DebugHelpers.findSetReg(prototype, pc, register);
if (pc == -1) return null;

var insn = prototype.code[pc];
return switch (GET_OPCODE(insn)) {
case OP_MOVE -> {
var a = GETARG_A(insn);
var b = GETARG_B(insn); // move from `b' to `a'
yield b < a ? resolveValueSource(state, frame, prototype, pc, register, depth + 1) : null; // Resolve 'b' .
}
case OP_GETTABUP, OP_GETTABLE, OP_SELF -> {
var tableIndex = GETARG_B(insn);
var keyIndex = GETARG_C(insn);
// We're only interested in expressions of the form "foo.bar". Showing a "did you mean" hint for
// "foo[i]" isn't very useful!
if (!ISK(keyIndex)) yield null;

var key = prototype.constants[INDEXK(keyIndex)];
if (key.type() != Constants.TSTRING) yield null;

var table = GET_OPCODE(insn) == OP_GETTABUP
? frame.closure.getUpvalue(tableIndex).getValue()
: evaluate(state, frame, prototype, pc, tableIndex, depth);
if (table == null) yield null;

var isGlobal = GET_OPCODE(insn) == OP_GETTABUP && Objects.equals(prototype.getUpvalueName(tableIndex), Constants.ENV);
yield new ValueSource(isGlobal, table, (LuaString) key);
}
default -> null;
};
}

/**
* Attempt to reconstruct the value of a register.
*
* @param state The current Lua state.
* @param frame The current debug frame.
* @param prototype The current function
* @param pc The PC to evaluate at.
* @param register The register to evaluate.
* @param depth The current depth. Starts at 0, and aborts once reaching {@link #MAX_DEPTH}.
* @return The reconstructed value, or {@code null} if unavailable.
*/
@SuppressWarnings("NullTernary")
private static @Nullable LuaValue evaluate(LuaState state, DebugFrame frame, Prototype prototype, int pc, int register, int depth) {
if (depth >= MAX_DEPTH) return null;

// If this is a local, then return its contents.
if (prototype.getLocalName(register + 1, pc) != null) return frame.stack[register];

// Otherwise find where this register was set. If unknown, then abort.
pc = DebugHelpers.findSetReg(prototype, pc, register);
if (pc == -1) return null;

var insn = prototype.code[pc];
var opcode = GET_OPCODE(insn);
return switch (opcode) {
case OP_MOVE -> {
var a = GETARG_A(insn);
var b = GETARG_B(insn); // move from `b' to `a'
yield b < a ? evaluate(state, frame, prototype, pc, register, depth + 1) : null; // Resolve 'b'.
}
// Load constants
case OP_LOADK -> prototype.constants[GETARG_Bx(insn)];
case OP_LOADKX -> prototype.constants[GETARG_Ax(prototype.code[pc + 1])];
case OP_LOADBOOL -> GETARG_B(insn) == 0 ? Constants.FALSE : Constants.TRUE;
case OP_LOADNIL -> Constants.NIL;
// Upvalues and tables.
case OP_GETUPVAL -> frame.closure.getUpvalue(GETARG_B(insn)).getValue();
case OP_GETTABLE, OP_GETTABUP -> {
var table = opcode == OP_GETTABUP
? frame.closure.getUpvalue(GETARG_B(insn)).getValue()
: evaluate(state, frame, prototype, pc, GETARG_B(insn), depth + 1);
if (table == null) yield null;

var key = evaluateK(state, frame, prototype, pc, GETARG_C(insn), depth + 1);
yield key == null ? null : safeIndex(state, table, key);
}
default -> null;
};
}

private static @Nullable LuaValue evaluateK(LuaState state, DebugFrame frame, Prototype prototype, int pc, int registerOrConstant, int depth) {
return ISK(registerOrConstant) ? prototype.constants[INDEXK(registerOrConstant)] : evaluate(state, frame, prototype, pc, registerOrConstant, depth + 1);
}

private static @Nullable LuaValue safeIndex(LuaState state, LuaValue table, LuaValue key) {
var loop = 0;
do {
LuaValue metatable;
if (table instanceof LuaTable tbl) {
var res = tbl.rawget(key);
if (!res.isNil() || (metatable = tbl.metatag(state, CachedMetamethod.INDEX)).isNil()) return res;
} else if ((metatable = table.metatag(state, CachedMetamethod.INDEX)).isNil()) {
return null;
}

if (metatable instanceof LuaFunction) return null;

table = metatable;
}
while (++loop < Constants.MAXTAGLOOP);

return null;
}
}
Loading

0 comments on commit b185d08

Please sign in to comment.