From 4ee02ccaca655c70529b7894f782983d5fe95dcd Mon Sep 17 00:00:00 2001 From: Toine Hartman Date: Tue, 24 Dec 2024 17:46:17 +0100 Subject: [PATCH 1/5] Add test demonstrating multi-module kw bug. --- .../src/main/rascal/lang/rascal/tests/rename/Fields.rsc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc b/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc index eff44aefc..ebc316af8 100644 --- a/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc +++ b/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc @@ -50,6 +50,14 @@ test bool commonKeywordField() = testRenameOccurrences({0, 1, 2, 3}, " ", decls = "data D(int foo = 0, int baz = 0) = d();" ); +test bool commonKeywordFieldFromOtherModule() = testRenameOccurrences({ + byText("Foo", "data D(int foo = 0, int baz = 0) = d();", {0}) + , byText("Bar", + "import Foo; + 'D oneTwo = d(foo=1, baz=2); + ", {0}) +}); + test bool multipleConstructorField() = testRenameOccurrences({0, 1, 2, 3}, " 'x = d(1, 2); 'y = x.foo; From cb4e3ce23d2698446df658f21ffc1661ac1237b6 Mon Sep 17 00:00:00 2001 From: Toine Hartman Date: Tue, 24 Dec 2024 17:48:17 +0100 Subject: [PATCH 2/5] Start working on fix. --- .../lang/rascal/lsp/refactor/Rename.rsc | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc index eb8a9d26e..d25c7a45f 100644 --- a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc +++ b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc @@ -306,10 +306,11 @@ private bool rascalIsFunctionLocal(TModel ws, cursor(use(), cursorLoc, _)) = private bool rascalIsFunctionLocal(TModel _, cursor(typeParam(), _, _)) = true; private default bool rascalIsFunctionLocal(_, _) = false; -Maybe[AType] rascalAdtCommonKeywordFieldType(TModel ws, str fieldName, Define _:<_, _, _, _, _, DefInfo defInfo>) { +Maybe[AType] rascalAdtCommonKeywordFieldType(TModel ws, str fieldName, Define _:<_, _, _, _, _, DefInfo defInfo>, PathConfig(loc) getPathConfig) { if (defInfo.commonKeywordFields? && kwf:(KeywordFormal) ` = ` <- defInfo.commonKeywordFields && "" == fieldName) { + ws = (ws | appendTModel(it, tm) | tm <- rascalTModels({kwf.src.top}, getPathConfig(kwf.src.top))); if (ft:just(_) := getFact(ws, kwf.src)) return ft; throw "Unknown field type for "; } @@ -326,10 +327,10 @@ Maybe[AType] rascalConsFieldType(str fieldName, Define _:<_, _, _, constructorId return nothing(); } -private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cursorLoc, str cursorName) { +private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cursorLoc, str cursorName, PathConfig(loc) getPathConfig) { for (Define dt <- rascalGetADTDefinitions(ws, container) && AType adtType := dt.defInfo.atype) { - if (just(fieldType) := rascalAdtCommonKeywordFieldType(ws, cursorName, dt)) { + if (just(fieldType) := rascalAdtCommonKeywordFieldType(ws, cursorName, dt, getPathConfig)) { // Case 4 or 5 (or 0): common keyword field return dataCommonKeywordField(dt.defined, fieldType); } @@ -353,7 +354,7 @@ private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cu throw illegalRename("Cannot rename \'\'; it is not defined in this workspace", {definitionsOutsideWorkspace(fromDefs)}); } -private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, rel[loc l, CursorKind kind] locsContainingCursor, rel[loc field, loc container] fields, rel[loc kw, loc container] keywords) { +private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, rel[loc l, CursorKind kind] locsContainingCursor, rel[loc field, loc container] fields, rel[loc kw, loc container] keywords, PathConfig(loc) getPathConfig) { loc c = min(locsContainingCursor.l); switch (locsContainingCursor[c]) { case {moduleName(), *_}: { @@ -361,7 +362,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, } case {keywordParam(), dataKeywordField(_, _), *_}: { if ({loc container} := keywords[c]) { - return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName); + return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName, getPathConfig); } } case {collectionField(), dataField(_, _), dataKeywordField(_, _), dataCommonKeywordField(_, _), *_}: { @@ -380,7 +381,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, // Case 1 (or 0): collection field return collectionField(); } - return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName); + return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName, getPathConfig); } } case {def(), *_}: { @@ -389,7 +390,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, if (d.idRole is fieldId && Define adt: <_, _, _, dataId(), _, _> <- ws.defines && isStrictlyContainedIn(c, adt.defined)) { - return rascalGetDataFieldCursorKind(ws, adt.defined, cursorLoc, cursorName); + return rascalGetDataFieldCursorKind(ws, adt.defined, cursorLoc, cursorName, getPathConfig); } return def(); } @@ -424,7 +425,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, throw unsupportedRename("Could not retrieve information for \'\' at ."); } -private Cursor rascalGetCursor(TModel ws, Tree cursorT) { +private Cursor rascalGetCursor(TModel ws, Tree cursorT, PathConfig(loc) getPathConfig) { loc cursorLoc = cursorT.src; str cursorName = ""; @@ -473,7 +474,7 @@ private Cursor rascalGetCursor(TModel ws, Tree cursorT) { throw unsupportedRename("Renaming \'\' at is not supported."); } - CursorKind kind = rascalGetCursorKind(ws, cursorLoc, cursorName, locsContainingCursor, fields, keywords); + CursorKind kind = rascalGetCursorKind(ws, cursorLoc, cursorName, locsContainingCursor, fields, keywords, getPathConfig); return cursor(kind, min(locsContainingCursor.l), cursorName); } @@ -607,7 +608,7 @@ Edits rascalRenameSymbol(Tree cursorT, set[loc] workspaceFolders, str newName, P TModel ws = loadLocs(tmodel(), preloadFiles(workspaceFolders, cursorLoc), localTmodelsForFiles); step("analyzing name at cursor", 1); - cur = rascalGetCursor(ws, cursorT); + cur = rascalGetCursor(ws, cursorT, getPathConfig); step("loading required type information", 1); if (!rascalIsFunctionLocal(ws, cur)) { From a2a9a3c1822a3c1eec1c7837661e5ce116a85a18 Mon Sep 17 00:00:00 2001 From: Toine Hartman Date: Wed, 8 Jan 2025 13:57:28 +0100 Subject: [PATCH 3/5] Fix renaming of keyword fields across modules. --- .../rascal/lsp/refactor/WorkspaceInfo.rsc | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc index bcc645891..e58572640 100644 --- a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc +++ b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/WorkspaceInfo.rsc @@ -383,8 +383,7 @@ set[RenameLocation] rascalGetKeywordFieldUses(TModel ws, set[loc] defs, str curs , isStrictlyContainedIn(consDef.defined, dataDef.defined) ) { if (AType fieldType := ws.definitions[d].defInfo.atype) { - set[loc] reachableModules = rascalReachableModules(ws, {d}); - if (<{}, consUses, _> := rascalGetFieldDefsUses(ws, reachableModules, dataDef.defInfo.atype, fieldType, cursorName)) { + if (<{}, consUses, _> := rascalGetFieldDefsUses(ws, d, dataDef.defInfo.atype, fieldType, cursorName)) { uses += consUses; } } else { @@ -571,14 +570,14 @@ DefsUsesRenames rascalGetDefsUses(TModel ws, cursor(cursorKind, cursorLoc, curso set[Define] reachableDefs = rascalReachableDefs(ws, adtDefs); set[loc] reachableModules = rascalReachableModules(ws, reachableDefs.defined); - for (Define _:<_, _, _, constructorId(), _, defType(AType consType)> <- reachableDefs) { - = rascalGetFieldDefsUses(ws, reachableModules, consType, cursorKind.fieldType, cursorName); + for (Define d:<_, _, _, constructorId(), _, defType(AType consType)> <- reachableDefs) { + = rascalGetFieldDefsUses(ws, d.defined, consType, cursorKind.fieldType, cursorName); defs += ds; uses += us; } - for (Define _:<_, _, _, IdRole idRole, _, defType(acons(AType dataType, _, _))> <- reachableDefs - , idRole != dataId()) { - = rascalGetFieldDefsUses(ws, reachableModules, dataType, cursorKind.fieldType, cursorName); + for (Define d:<_, _, _, _, _, defType(acons(AType dataType, _, _))> <- reachableDefs + , d.idRole != dataId()) { + = rascalGetFieldDefsUses(ws, d.defined, dataType, cursorKind.fieldType, cursorName); defs += ds; uses += us; } @@ -609,14 +608,14 @@ DefsUsesRenames rascalGetDefsUses(TModel ws, cursor(collectionField(), cursorLoc , rascalIsCollectionType(collUseType) , collUseType.elemType is atypeList) { // We are at a collection definition site - return rascalGetFieldDefsUses(ws, rascalReachableModules(ws, {cursorLoc}), collUseType, cursorType, cursorName); + return rascalGetFieldDefsUses(ws, cursorLoc, collUseType, cursorType, cursorName); } // We can find the collection type by looking for the first use to the left of the cursor that has a collection type lrel[loc use, loc def] usesToLeft = reverse(sort({ | <- ws.useDef, isSameFile(u, cursorLoc), u.offset < cursorLoc.offset})); if (<_, d> <- usesToLeft, define := ws.definitions[d], defType(AType collDefType) := define.defInfo, rascalIsCollectionType(collDefType)) { // We are at a use site, where the field element type is wrapped in a `aset` of `alist` constructor - return rascalGetFieldDefsUses(ws, rascalReachableModules(ws, {define.defined}), collDefType, isTupleField(cursorType) ? cursorType.elmType : cursorType, cursorName); + return rascalGetFieldDefsUses(ws, define.defined, collDefType, isTupleField(cursorType) ? cursorType.elmType : cursorType, cursorName); } else { throw unsupportedRename("Could not find a collection definition corresponding to the field at the cursor."); } @@ -666,8 +665,8 @@ Maybe[tuple[loc, set[loc], bool]] rascalGetKeywordLocs(str fieldName, d:(Declara default Maybe[tuple[loc, set[loc], bool]] rascalGetKeywordLocs(str _, Tree _) = nothing(); -private DefsUsesRenames rascalGetFieldDefsUses(TModel ws, set[loc] reachableModules, AType containerType, AType fieldType, str cursorName) { - set[loc] containerFacts = {f | f <- factsInvert(ws)[containerType], f.top in reachableModules}; +private DefsUsesRenames rascalGetFieldDefsUses(TModel ws, loc consOrDataDef, AType containerType, AType fieldType, str cursorName) { + set[loc] containerFacts = {f | f <- factsInvert(ws)[containerType], consOrDataDef.top in rascalReachableModules(ws, {f})}; rel[loc file, loc u] factsByModule = groupBy(containerFacts, loc(loc l) { return l.top; }); set[loc] defs = {}; From 2ab82def5768f9ef2c4a3481d5c8068b66989e5d Mon Sep 17 00:00:00 2001 From: Toine Hartman Date: Wed, 8 Jan 2025 14:06:58 +0100 Subject: [PATCH 4/5] Add similar test for constructor kw field. --- .../src/main/rascal/lang/rascal/tests/rename/Fields.rsc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc b/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc index ebc316af8..f396bd81b 100644 --- a/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc +++ b/rascal-lsp/src/main/rascal/lang/rascal/tests/rename/Fields.rsc @@ -43,6 +43,14 @@ test bool constructorKeywordField() = testRenameOccurrences({0, 1, 2, 3}, " ", decls="data D = d(int foo = 0, int baz = 0);" ); +test bool constructorKeywordFieldFromOtherModule() = testRenameOccurrences({ + byText("Foo", "data D = d(int foo = 0, int baz = 0);", {0}) + , byText("Bar", + "import Foo; + 'D oneTwo = d(foo=1, baz=2); + ", {0}) +}); + test bool commonKeywordField() = testRenameOccurrences({0, 1, 2, 3}, " 'D oneTwo = d(foo=1, baz=2); 'x = oneTwo.foo; From 9929df708bdddf194da23979ab33828443c47560 Mon Sep 17 00:00:00 2001 From: Toine Hartman Date: Wed, 8 Jan 2025 14:10:32 +0100 Subject: [PATCH 5/5] Revert "Start working on fix." This reverts commit cb4e3ce23d2698446df658f21ffc1661ac1237b6. --- .../lang/rascal/lsp/refactor/Rename.rsc | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc index d25c7a45f..eb8a9d26e 100644 --- a/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc +++ b/rascal-lsp/src/main/rascal/lang/rascal/lsp/refactor/Rename.rsc @@ -306,11 +306,10 @@ private bool rascalIsFunctionLocal(TModel ws, cursor(use(), cursorLoc, _)) = private bool rascalIsFunctionLocal(TModel _, cursor(typeParam(), _, _)) = true; private default bool rascalIsFunctionLocal(_, _) = false; -Maybe[AType] rascalAdtCommonKeywordFieldType(TModel ws, str fieldName, Define _:<_, _, _, _, _, DefInfo defInfo>, PathConfig(loc) getPathConfig) { +Maybe[AType] rascalAdtCommonKeywordFieldType(TModel ws, str fieldName, Define _:<_, _, _, _, _, DefInfo defInfo>) { if (defInfo.commonKeywordFields? && kwf:(KeywordFormal) ` = ` <- defInfo.commonKeywordFields && "" == fieldName) { - ws = (ws | appendTModel(it, tm) | tm <- rascalTModels({kwf.src.top}, getPathConfig(kwf.src.top))); if (ft:just(_) := getFact(ws, kwf.src)) return ft; throw "Unknown field type for "; } @@ -327,10 +326,10 @@ Maybe[AType] rascalConsFieldType(str fieldName, Define _:<_, _, _, constructorId return nothing(); } -private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cursorLoc, str cursorName, PathConfig(loc) getPathConfig) { +private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cursorLoc, str cursorName) { for (Define dt <- rascalGetADTDefinitions(ws, container) && AType adtType := dt.defInfo.atype) { - if (just(fieldType) := rascalAdtCommonKeywordFieldType(ws, cursorName, dt, getPathConfig)) { + if (just(fieldType) := rascalAdtCommonKeywordFieldType(ws, cursorName, dt)) { // Case 4 or 5 (or 0): common keyword field return dataCommonKeywordField(dt.defined, fieldType); } @@ -354,7 +353,7 @@ private CursorKind rascalGetDataFieldCursorKind(TModel ws, loc container, loc cu throw illegalRename("Cannot rename \'\'; it is not defined in this workspace", {definitionsOutsideWorkspace(fromDefs)}); } -private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, rel[loc l, CursorKind kind] locsContainingCursor, rel[loc field, loc container] fields, rel[loc kw, loc container] keywords, PathConfig(loc) getPathConfig) { +private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, rel[loc l, CursorKind kind] locsContainingCursor, rel[loc field, loc container] fields, rel[loc kw, loc container] keywords) { loc c = min(locsContainingCursor.l); switch (locsContainingCursor[c]) { case {moduleName(), *_}: { @@ -362,7 +361,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, } case {keywordParam(), dataKeywordField(_, _), *_}: { if ({loc container} := keywords[c]) { - return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName, getPathConfig); + return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName); } } case {collectionField(), dataField(_, _), dataKeywordField(_, _), dataCommonKeywordField(_, _), *_}: { @@ -381,7 +380,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, // Case 1 (or 0): collection field return collectionField(); } - return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName, getPathConfig); + return rascalGetDataFieldCursorKind(ws, container, cursorLoc, cursorName); } } case {def(), *_}: { @@ -390,7 +389,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, if (d.idRole is fieldId && Define adt: <_, _, _, dataId(), _, _> <- ws.defines && isStrictlyContainedIn(c, adt.defined)) { - return rascalGetDataFieldCursorKind(ws, adt.defined, cursorLoc, cursorName, getPathConfig); + return rascalGetDataFieldCursorKind(ws, adt.defined, cursorLoc, cursorName); } return def(); } @@ -425,7 +424,7 @@ private CursorKind rascalGetCursorKind(TModel ws, loc cursorLoc, str cursorName, throw unsupportedRename("Could not retrieve information for \'\' at ."); } -private Cursor rascalGetCursor(TModel ws, Tree cursorT, PathConfig(loc) getPathConfig) { +private Cursor rascalGetCursor(TModel ws, Tree cursorT) { loc cursorLoc = cursorT.src; str cursorName = ""; @@ -474,7 +473,7 @@ private Cursor rascalGetCursor(TModel ws, Tree cursorT, PathConfig(loc) getPathC throw unsupportedRename("Renaming \'\' at is not supported."); } - CursorKind kind = rascalGetCursorKind(ws, cursorLoc, cursorName, locsContainingCursor, fields, keywords, getPathConfig); + CursorKind kind = rascalGetCursorKind(ws, cursorLoc, cursorName, locsContainingCursor, fields, keywords); return cursor(kind, min(locsContainingCursor.l), cursorName); } @@ -608,7 +607,7 @@ Edits rascalRenameSymbol(Tree cursorT, set[loc] workspaceFolders, str newName, P TModel ws = loadLocs(tmodel(), preloadFiles(workspaceFolders, cursorLoc), localTmodelsForFiles); step("analyzing name at cursor", 1); - cur = rascalGetCursor(ws, cursorT, getPathConfig); + cur = rascalGetCursor(ws, cursorT); step("loading required type information", 1); if (!rascalIsFunctionLocal(ws, cur)) {