Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix string interpolation autocomplete and location #748

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions Analysis/src/Autocomplete.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1219,6 +1219,31 @@ static std::optional<const ClassTypeVar*> getMethodContainingClass(const ModuleP
return std::nullopt;
}

static bool stringPartOfInterpString(const AstNode* node, Position position)
{
const AstExprInterpString* interpString = node->as<AstExprInterpString>();
if (!interpString)
{
return false;
}

for (const AstExpr* expression : interpString->expressions)
{
if (expression->location.containsClosed(position))
{
return false;
}
}

return true;
}

static bool isSimpleInterpolatedString(const AstNode* node)
{
const AstExprInterpString* interpString = node->as<AstExprInterpString>();
return interpString != nullptr && interpString->expressions.size == 0;
}

static std::optional<AutocompleteEntryMap> autocompleteStringParams(const SourceModule& sourceModule, const ModulePtr& module,
const std::vector<AstNode*>& nodes, Position position, StringCompletionCallback callback)
{
Expand All @@ -1227,7 +1252,7 @@ static std::optional<AutocompleteEntryMap> autocompleteStringParams(const Source
return std::nullopt;
}

if (!nodes.back()->is<AstExprConstantString>() && !nodes.back()->is<AstExprError>())
if (!nodes.back()->is<AstExprConstantString>() && !isSimpleInterpolatedString(nodes.back()) && !nodes.back()->is<AstExprError>())
{
return std::nullopt;
}
Expand Down Expand Up @@ -1432,7 +1457,7 @@ static AutocompleteResult autocomplete(const SourceModule& sourceModule, const M
return autocompleteExpression(sourceModule, *module, singletonTypes, &typeArena, ancestry, position);
else if (AstStatRepeat* statRepeat = extractStat<AstStatRepeat>(ancestry); statRepeat)
return {autocompleteStatement(sourceModule, *module, ancestry, position), ancestry, AutocompleteContext::Statement};
else if (AstExprTable* exprTable = parent->as<AstExprTable>(); exprTable && (node->is<AstExprGlobal>() || node->is<AstExprConstantString>()))
else if (AstExprTable* exprTable = parent->as<AstExprTable>(); exprTable && (node->is<AstExprGlobal>() || node->is<AstExprConstantString>() || node->is<AstExprInterpString>()))
{
for (const auto& [kind, key, value] : exprTable->items)
{
Expand Down Expand Up @@ -1471,7 +1496,7 @@ static AutocompleteResult autocomplete(const SourceModule& sourceModule, const M
{
return {*ret, ancestry, AutocompleteContext::String};
}
else if (node->is<AstExprConstantString>())
else if (node->is<AstExprConstantString>() || isSimpleInterpolatedString(node))
{
AutocompleteEntryMap result;

Expand All @@ -1497,6 +1522,13 @@ static AutocompleteResult autocomplete(const SourceModule& sourceModule, const M

return {result, ancestry, AutocompleteContext::String};
}
else if (stringPartOfInterpString(node, position))
{
// We're not a simple interpolated string, we're something like `a{"b"}@1`, and we
// can't know what to format to
AutocompleteEntryMap map;
return {map, ancestry, AutocompleteContext::String};
}

if (node->is<AstExprConstantNumber>())
return {};
Expand Down
50 changes: 38 additions & 12 deletions Ast/src/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2661,23 +2661,24 @@ AstExpr* Parser::parseInterpString()
TempVector<AstExpr*> expressions(scratchExpr);

Location startLocation = lexer.current().location;
Location endLocation;

do
{
Lexeme currentLexeme = lexer.current();
LUAU_ASSERT(currentLexeme.type == Lexeme::InterpStringBegin || currentLexeme.type == Lexeme::InterpStringMid ||
currentLexeme.type == Lexeme::InterpStringEnd || currentLexeme.type == Lexeme::InterpStringSimple);

Location location = currentLexeme.location;
endLocation = currentLexeme.location;

Location startOfBrace = Location(location.end, 1);
Location startOfBrace = Location(endLocation.end, 1);

scratchData.assign(currentLexeme.data, currentLexeme.length);

if (!Lexer::fixupQuotedString(scratchData))
{
nextLexeme();
return reportExprError(startLocation, {}, "Interpolated string literal contains malformed escape sequence");
return reportExprError(Location{startLocation, endLocation}, {}, "Interpolated string literal contains malformed escape sequence");
}

AstArray<char> chars = copy(scratchData);
Expand All @@ -2688,15 +2689,36 @@ AstExpr* Parser::parseInterpString()

if (currentLexeme.type == Lexeme::InterpStringEnd || currentLexeme.type == Lexeme::InterpStringSimple)
{
AstArray<AstArray<char>> stringsArray = copy(strings);
AstArray<AstExpr*> expressionsArray = copy(expressions);

return allocator.alloc<AstExprInterpString>(startLocation, stringsArray, expressionsArray);
break;
}

AstExpr* expression = parseExpr();
bool errorWhileChecking = false;

switch (lexer.current().type)
{
case Lexeme::InterpStringMid:
case Lexeme::InterpStringEnd:
{
errorWhileChecking = true;
nextLexeme();
expressions.push_back(reportExprError(endLocation, {}, "Malformed interpolated string, expected expression inside '{}'"));
break;
}
case Lexeme::BrokenString:
{
errorWhileChecking = true;
nextLexeme();
expressions.push_back(reportExprError(endLocation, {}, "Malformed interpolated string, did you forget to add a '`'?"));
break;
}
default:
expressions.push_back(parseExpr());
}

expressions.push_back(expression);
if (errorWhileChecking)
{
break;
}

switch (lexer.current().type)
{
Expand All @@ -2706,14 +2728,18 @@ AstExpr* Parser::parseInterpString()
break;
case Lexeme::BrokenInterpDoubleBrace:
nextLexeme();
return reportExprError(location, {}, ERROR_INVALID_INTERP_DOUBLE_BRACE);
return reportExprError(endLocation, {}, ERROR_INVALID_INTERP_DOUBLE_BRACE);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: note for some future clean-up PR, constant should have been a static const char* kErrorInvalidInterpDoubleBrace = ; instead of a define.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For whatever reason, that didn't work when I tried it.

https://github.com/Roblox/luau/pull/614/files#r931152226

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another work-around would be reportExprError(endLocation, {}, "%s", kErrorInvalidInterpDoubleBrace); but doesn't matter any more.

case Lexeme::BrokenString:
nextLexeme();
return reportExprError(location, {}, "Malformed interpolated string, did you forget to add a '}'?");
return reportExprError(endLocation, {}, "Malformed interpolated string, did you forget to add a '}'?");
default:
return reportExprError(location, {}, "Malformed interpolated string, got %s", lexer.current().toString().c_str());
return reportExprError(endLocation, {}, "Malformed interpolated string, got %s", lexer.current().toString().c_str());
}
} while (true);

AstArray<AstArray<char>> stringsArray = copy(strings);
AstArray<AstExpr*> expressionsArray = copy(expressions);
return allocator.alloc<AstExprInterpString>(Location{startLocation, endLocation}, stringsArray, expressionsArray);
}

AstExpr* Parser::parseNumber()
Expand Down
2 changes: 1 addition & 1 deletion tests/AstJsonEncoder.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ TEST_CASE_FIXTURE(JsonEncoderFixture, "encode_AstExprInterpString")
AstStat* statement = expectParseStatement("local a = `var = {x}`");

std::string_view expected =
R"({"type":"AstStatLocal","location":"0,0 - 0,18","vars":[{"luauType":null,"name":"a","type":"AstLocal","location":"0,6 - 0,7"}],"values":[{"type":"AstExprInterpString","location":"0,10 - 0,18","strings":["var = ",""],"expressions":[{"type":"AstExprGlobal","location":"0,18 - 0,19","global":"x"}]}]})";
R"({"type":"AstStatLocal","location":"0,0 - 0,21","vars":[{"luauType":null,"name":"a","type":"AstLocal","location":"0,6 - 0,7"}],"values":[{"type":"AstExprInterpString","location":"0,10 - 0,21","strings":["var = ",""],"expressions":[{"type":"AstExprGlobal","location":"0,18 - 0,19","global":"x"}]}]})";

CHECK(toJson(statement) == expected);
}
Expand Down
67 changes: 66 additions & 1 deletion tests/Autocomplete.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "Luau/StringUtils.h"

#include "Fixture.h"
#include "ScopedFlags.h"

#include "doctest.h"

Expand Down Expand Up @@ -2708,13 +2709,77 @@ a = if temp then even else abc@3
CHECK(ac.entryMap.count("abcdef"));
}

TEST_CASE_FIXTURE(ACFixture, "autocomplete_interpolated_string")
TEST_CASE_FIXTURE(ACFixture, "autocomplete_interpolated_string_constant")
{
ScopedFastFlag sff{"LuauInterpolatedStringBaseSupport", true};

check(R"(f(`@1`))");
auto ac = autocomplete('1');
CHECK(ac.entryMap.empty());
CHECK_EQ(ac.context, AutocompleteContext::String);

check(R"(f(`@1 {"a"}`))");
ac = autocomplete('1');
CHECK(ac.entryMap.empty());
CHECK_EQ(ac.context, AutocompleteContext::String);

check(R"(f(`{"a"} @1`))");
ac = autocomplete('1');
CHECK(ac.entryMap.empty());
CHECK_EQ(ac.context, AutocompleteContext::String);

check(R"(f(`{"a"} @1 {"b"}`))");
ac = autocomplete('1');
CHECK(ac.entryMap.empty());
CHECK_EQ(ac.context, AutocompleteContext::String);
}

TEST_CASE_FIXTURE(ACFixture, "autocomplete_interpolated_string_expression")
{
ScopedFastFlag sff{"LuauInterpolatedStringBaseSupport", true};

check(R"(f(`expression = {@1}`))");
auto ac = autocomplete('1');
CHECK(ac.entryMap.count("table"));
CHECK_EQ(ac.context, AutocompleteContext::Expression);
}

TEST_CASE_FIXTURE(ACFixture, "autocomplete_interpolated_string_expression_with_comments")
{
ScopedFastFlag sff{"LuauInterpolatedStringBaseSupport", true};

check(R"(f(`expression = {--[[ bla bla bla ]]@1`))");

auto ac = autocomplete('1');
CHECK(ac.entryMap.count("table"));
CHECK_EQ(ac.context, AutocompleteContext::Expression);

check(R"(f(`expression = {@1 --[[ bla bla bla ]]`))");
ac = autocomplete('1');
CHECK(!ac.entryMap.empty());
CHECK(ac.entryMap.count("table"));
CHECK_EQ(ac.context, AutocompleteContext::Expression);
}

TEST_CASE_FIXTURE(ACFixture, "autocomplete_interpolated_string_as_singleton")
{
ScopedFastFlag sff{"LuauInterpolatedStringBaseSupport", true};

check(R"(
--!strict
local function f(a: "cat" | "dog") end

f(`@1`)
f(`uhhh{'try'}@2`)
)");

auto ac = autocomplete('1');
CHECK(ac.entryMap.count("cat"));
CHECK_EQ(ac.context, AutocompleteContext::String);

ac = autocomplete('2');
CHECK(ac.entryMap.empty());
CHECK_EQ(ac.context, AutocompleteContext::String);
}

TEST_CASE_FIXTURE(ACFixture, "autocomplete_explicit_type_pack")
Expand Down