Merge pull request #711 from usethesource/fix/708-rename-constructor-names

toinehartman · web-flow · commit fb9bf7949c7a · 2025-08-29T13:22:33.000+02:00
Improve constructor rename checks
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/Rename.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/Rename.rsc
@@ -117,9 +117,9 @@ void rascalCheckCausesOverlappingDefinitions(set[Define] currentDefs, str newNam
     }
 }
 
-void rascalCheckLegalNameByRole(Define _:<_, _, _, role, at, _>, str name, Renamer r) {
+void rascalCheckLegalNameByRole(Define _:<_, _, _, role, at, dt>, str name, Renamer r) {
     escName = normalizeEscaping(name);
-    <t, desc> = asType(role);
+    <t, desc> = asType(role, dt);
     if (tryParseAs(t, escName) is nothing) {
         r.error(at, "<escName> is not a valid <desc>");
     }
@@ -336,9 +336,9 @@ set[Define] getCursorDefinitions(list[Tree] cursor, Tree(loc) _, TModel(Tree) _,
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFiles(set[Define] defs, list[Tree] cursor, str newName, Tree(loc) getTree, Renamer r) {
     escNewName = normalizeEscaping(newName);
-    for (role <- defs.idRole) {
+    for (<role, dt> <- defs<idRole, defInfo>) {
         hasError = false;
-        <t, desc> = asType(role);
+        <t, desc> = asType(role, dt);
         if (tryParseAs(t, escNewName) is nothing) {
             hasError = true;
             r.error(cursor[0], "\'<escNewName>\' is not a valid <desc>");
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Constructors.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Constructors.rsc
@@ -77,4 +77,5 @@ set[Define] findAdditionalConstructorDefinitions(set[Define] cursorDefs, Tree tr
     });
 }
 
-tuple[type[Tree] as, str desc] asType(constructorId()) = <#NonterminalLabel, "constructor name">;
+// ADT constructors
+tuple[type[Tree] as, str desc] asType(constructorId(), defType(acons(aadt(_, _, dataSyntax()), _, _))) = <#Name, "data constructor name">;
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Fields.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Fields.rsc
@@ -143,10 +143,10 @@ TModel augmentFieldUses(Tree tr, TModel tm, TModel(loc) getModel) {
 }
 
 // Positional fields
-tuple[type[Tree] as, str desc] asType(fieldId()) = <#NonterminalLabel, "field name">;
+tuple[type[Tree] as, str desc] asType(fieldId(), _) = <#NonterminalLabel, "field name">;
 
 // Keyword fields
-tuple[type[Tree] as, str desc] asType(keywordFieldId()) = <#Name, "keyword field name">;
+tuple[type[Tree] as, str desc] asType(keywordFieldId(), _) = <#Name, "keyword field name">;
 
 bool isUnsupportedCursor(list[Tree] _: [*_, Name n1, *_, (Expression) `<Expression _> has <Name n2>`, *_], Renamer _) = (n1 := n2);
 
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Functions.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Functions.rsc
@@ -54,4 +54,4 @@ set[Define] findAdditionalDefinitions(set[Define] cursorDefs:{<_, _, _, function
 set[Define] findAdditionalFunctionDefinitions(set[Define] cursorDefs, TModel tm) =
     {tm.definitions[d] | loc d <- (tm.defines<idRole, defined>)[functionId()], rascalMayOverloadSameName(cursorDefs.defined + d, tm.definitions)};
 
-tuple[type[Tree] as, str desc] asType(functionId()) = <#Name, "function name">;
+tuple[type[Tree] as, str desc] asType(functionId(), _) = <#Name, "function name">;
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Grammars.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Grammars.rsc
@@ -61,10 +61,14 @@ void renameDefinitionUnchecked(Define d: <_, _, _, lexicalId(), _, _>, loc _, st
 }
 
 // Non-terminals
-tuple[type[Tree] as, str desc] asType(nonterminalId()) = <#Nonterminal, "production name">;
+tuple[type[Tree] as, str desc] asType(nonterminalId(), _) = <#Nonterminal, "production name">;
 
 // Lexicals
-tuple[type[Tree] as, str desc] asType(lexicalId()) = <#Nonterminal, "production name">;
+tuple[type[Tree] as, str desc] asType(lexicalId(), _) = <#Nonterminal, "production name">;
+
+// Grammar constructors
+tuple[type[Tree] as, str desc] asType(constructorId(), defType(acons(aadt(_, _, syntaxRole), _, _))) = <#NonterminalLabel, "grammar constructor name">
+    when !syntaxRole is dataSyntax;
 
 TModel augmentExceptProductions(Tree tr, TModel tm, TModel(loc) tmodelForLoc) {
     top-down-break visit (tr) {
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Modules.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Modules.rsc
@@ -47,7 +47,7 @@ import util::FileSystem;
 import util::Reflective;
 import util::Util;
 
-tuple[type[Tree] as, str desc] asType(moduleId()) = <#QualifiedName, "module name">;
+tuple[type[Tree] as, str desc] asType(moduleId(), _) = <#QualifiedName, "module name">;
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFilesUnchecked(set[Define] _:{<_, str defName, _, moduleId(), loc d, _>}, list[Tree] cursor, str newName, Tree(loc) getTree, Renamer r) {
     set[loc] useFiles = {};
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Parameters.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Parameters.rsc
@@ -41,7 +41,7 @@ import util::Maybe;
 
 bool isFormalId(IdRole role) = role in formalRoles;
 
-tuple[type[Tree] as, str desc] asType(IdRole idRole) = <#Name, "formal parameter name"> when isFormalId(idRole);
+tuple[type[Tree] as, str desc] asType(IdRole idRole, _) = <#Name, "formal parameter name"> when isFormalId(idRole);
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFilesUnchecked(set[Define] _:{<loc scope, _, _, IdRole role, _, _>}, list[Tree] cursor, str newName, Tree(loc) _, Renamer _) =
     <{scope.top}, {scope.top}, singleNameFilter(newName)(cursor[-1]) ? {scope.top} : {}>
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Types.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Types.rsc
@@ -86,10 +86,10 @@ public set[Define] findAdditionalDataLikeDefinitions(set[Define] defs, TModel tm
     };
 }
 
-tuple[type[Tree] as, str desc] asType(aliasId()) = <#Name, "type name">;
-tuple[type[Tree] as, str desc] asType(annoId()) = <#Name, "annotation name">;
-tuple[type[Tree] as, str desc] asType(dataId()) = <#Name, "ADT name">;
-tuple[type[Tree] as, str desc] asType(typeVarId()) = <#Name, "type variable name">;
+tuple[type[Tree] as, str desc] asType(aliasId(), _) = <#Name, "type name">;
+tuple[type[Tree] as, str desc] asType(annoId(), _) = <#Name, "annotation name">;
+tuple[type[Tree] as, str desc] asType(dataId(), _) = <#Name, "ADT name">;
+tuple[type[Tree] as, str desc] asType(typeVarId(), _) = <#Name, "type variable name">;
 
 alias Environment = tuple[TModel tm, map[str, loc] defs];
 
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Variables.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/lsp/refactor/rename/Variables.rsc
@@ -37,21 +37,21 @@ import analysis::typepal::TModel;
 import util::Maybe;
 
  // Variables
-tuple[type[Tree] as, str desc] asType(variableId()) = <#Name, "variable name">;
+tuple[type[Tree] as, str desc] asType(variableId(), _) = <#Name, "variable name">;
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFilesUnchecked(set[Define] _:{<loc scope, _, _, variableId(), _, _>}, list[Tree] cursor, str newName, Tree(loc) _, Renamer _) =
     <{scope.top}, {scope.top}, singleNameFilter(newName)(cursor[-1]) ? {scope.top} : {}>;
 
 // Global variables
-tuple[type[Tree] as, str desc] asType(moduleVariableId()) = <#Name, "variable name">;
+tuple[type[Tree] as, str desc] asType(moduleVariableId(), _) = <#Name, "variable name">;
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFilesUnchecked(set[Define] _:{<loc scope, _, _, moduleVariableId(), _, defType(_, vis=privateVis())>}, list[Tree] cursor, str newName, Tree(loc) getTree, Renamer r) {
     <curUseFiles, newFiles> = filterFiles(getSourceFiles(r), "<cursor[0]>", newName, getTree);
     return <{scope.top}, curUseFiles, newFiles>;
 }
 
 // Pattern variables
-tuple[type[Tree] as, str desc] asType(patternVariableId()) = <#Name, "pattern variable name">;
+tuple[type[Tree] as, str desc] asType(patternVariableId(), _) = <#Name, "pattern variable name">;
 
 tuple[set[loc], set[loc], set[loc]] findOccurrenceFilesUnchecked(set[Define] _:{<loc scope, _, _, patternVariableId(), _, _>}, list[Tree] cursor, str newName, Tree(loc) _, Renamer _) =
     <{scope.top}, {scope.top}, singleNameFilter(newName)(cursor[-1]) ? {scope.top} : {}>;
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/Constructors.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/Constructors.rsc
@@ -29,6 +29,14 @@ module lang::rascal::tests::rename::Constructors
 
 import lang::rascal::tests::rename::TestUtils;
 
+test bool constructorPascalName() = testRenameOccurrences({0}, "", decls ="
+    'data S = foo();
+", newName = "Foo");
+
+test bool constructorUnderscoreName() = testRenameOccurrences({0}, "", decls ="
+    'data S = foo();
+", newName = "_Foo");
+
 test bool extendedConstructor() = testRenameOccurrences({
     byText("Definer", "data Foo = foo(int i);", {0})
   , byText("Extender",
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/Grammars.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/Grammars.rsc
@@ -91,6 +91,15 @@ decls = "
     '  ;
 ");
 
+@expected{illegalRename}
+test bool exceptedConstructorInvalidName() = testRename("",
+decls = "
+    'syntax S
+    '  = foo: \"foo\" S s
+    '  | single: S s!foo
+    '  ;
+", newName = "Foo");
+
 test bool exceptConstructorDifferentNonterminal() = testRenameOccurrences({0, 1, 2}, "",
 decls = "
     'syntax S
@@ -159,12 +168,27 @@ test bool nonterminalInvalidName() = testRename("", decls ="
 ", oldName = "Foo", newName = "foo");
 
 @expected{illegalRename}
-test bool constructorInvalidName() = testRename("", decls ="
-    'syntax S
-    '   = foo: \"foo\"
-    '   ;
+test bool grammarConstructorPascalName() = testRenameOccurrences({0}, "", decls ="
+    'syntax S = foo: \"foo\";
 ", newName = "Foo");
 
+@expected{illegalRename}
+test bool grammarConstructorUnderscoreName() = testRenameOccurrences({0}, "", decls ="
+    'syntax S = foo: \"foo\";
+", newName = "_Foo");
+
+@expected{illegalRename}
+test bool labelPascalName() = testRename("", decls ="
+    'syntax S = s: \"s\";
+    'syntax T = \"withS\" S foo;
+", newName = "Foo");
+
+@expected{illegalRename}
+test bool labelUnderscoreName() = testRename("", decls ="
+    'syntax S = s: \"s\";
+    'syntax T = \"withS\" S foo;
+", newName = "_foo");
+
 @expected{illegalRename}
 test bool constructorFieldInvalidName() = testRename("", decls ="
     'syntax S
diff --git a/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/TestUtils.rsc b/rascal-lsp/src/main/rascal/lsp/lang/rascal/tests/rename/TestUtils.rsc
@@ -80,7 +80,7 @@ private void verifyTypeCorrectRenaming(loc root, list[DocumentEdit] edits, PathC
     assert size(editLocs) == size(toSet(editLocs)) : "Duplicate locations in suggested edits - VS Code cannot handle this";
 
     RascalCompilerConfig ccfg = rascalCompilerConfig(pcfg)[verbose = false][logPathConfig = false];
-    checkBefore = checkAll(root, ccfg);
+    list[ModuleMessages] checkBefore = checkAll(root, ccfg);
 
     // Back-up sources
     loc backupLoc = |memory://tests/backup|;
@@ -90,10 +90,12 @@ private void verifyTypeCorrectRenaming(loc root, list[DocumentEdit] edits, PathC
     executeDocumentEdits(sortEdits(groupEditsByFile(edits)));
     remove(pcfg.bin, recursive = true);
 
-    checkAfter = checkAll(root, ccfg);
+    list[ModuleMessages] checkAfter = checkAll(root, ccfg);
     newMsgs = checkAfter - checkBefore;
 
-    if (newErrors: [_, *_] := [m | m <- newMsgs, m is error]) throw newErrors;
+    if (newErrors: [_, *_] := [e | program(_, msgs) <- newMsgs, e <- msgs, e is error]) {
+        throw newErrors;
+    }
 
     // Restore back-up
     remove(root, recursive = true);

Original file line number	Diff line number	Diff line change
`@@ -77,4 +77,5 @@ set[Define] findAdditionalConstructorDefinitions(set[Define] cursorDefs, Tree tr`
`77`	`77`	`});`
`78`	`78`	`}`
`79`	`79`
`80`		`-tuple[type[Tree] as, str desc] asType(constructorId()) = <#NonterminalLabel, "constructor name">;`
	`80`	`+// ADT constructors`
	`81`	`+tuple[type[Tree] as, str desc] asType(constructorId(), defType(acons(aadt(_, _, dataSyntax()), _, _))) = <#Name, "data constructor name">;`
Original file line number	Diff line number	Diff line change
`@@ -143,10 +143,10 @@ TModel augmentFieldUses(Tree tr, TModel tm, TModel(loc) getModel) {`
`143`	`143`	`}`
`144`	`144`
`145`	`145`	`// Positional fields`
`146`		`-tuple[type[Tree] as, str desc] asType(fieldId()) = <#NonterminalLabel, "field name">;`
	`146`	`+tuple[type[Tree] as, str desc] asType(fieldId(), _) = <#NonterminalLabel, "field name">;`
`147`	`147`
`148`	`148`	`// Keyword fields`
`149`		`-tuple[type[Tree] as, str desc] asType(keywordFieldId()) = <#Name, "keyword field name">;`
	`149`	`+tuple[type[Tree] as, str desc] asType(keywordFieldId(), _) = <#Name, "keyword field name">;`
`150`	`150`
`151`	`151`	bool isUnsupportedCursor(list[Tree] _: [_, Name n1, _, (Expression) `<Expression _> has <Name n2>`, *_], Renamer _) = (n1 := n2);
`152`	`152`