Skip to content

Add more tests for RBS type aliases #839

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.

Sign up for GitHub

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

Open
jesse-shopify wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add more tests for RBS type aliases #839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 32 additions & 35 deletions rbs/CommentsAssociator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,10 @@ int CommentsAssociator::maybeInsertStandalonePlaceholders(parser::NodeVec &nodes
if (!typeAliasAllowedInContext()) {
if (auto e = ctx.beginError(it->second.loc, core::errors::Rewriter::RBSUnusedComment)) {
e.setHeader("Unexpected RBS type alias comment");
if (!contextAllowingTypeAlias.empty()) {
// Inside a method - show where the method is
auto loc = contextAllowingTypeAlias.back().second;
e.addErrorLine(ctx.locAt(loc),
"RBS type aliases are only allowed in class and module bodies. Found in:");
} else {
// At top level - no context to show
e.addErrorNote("RBS type aliases are only allowed in class and module bodies");
}
auto loc = contextAllowingTypeAlias.back().second;
e.addErrorLine(
ctx.locAt(loc),
"RBS type aliases are only allowed in class and module bodies, not in method bodies");
}

it = commentByLine.erase(it);
Expand Down Expand Up @@ -290,7 +285,7 @@ void CommentsAssociator::processTrailingComments(parser::Node *node, parser::Nod
unique_ptr<parser::Node> CommentsAssociator::walkBody(parser::Node *node, unique_ptr<parser::Node> body) {
ENFORCE(node != nullptr, "walkBody requires non-null node for location");

if (typeAliasAllowedInContext() && body == nullptr) {
if (body == nullptr) {
int endLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.endPos()).line;
auto nodes = parser::NodeVec();

Expand All @@ -304,16 +299,6 @@ unique_ptr<parser::Node> CommentsAssociator::walkBody(parser::Node *node, unique
return nullptr;
}

if (body == nullptr) {
auto nodes = parser::NodeVec();
processTrailingComments(node, nodes);

if (!nodes.empty()) {
return make_unique<parser::Begin>(node->loc, move(nodes));
}
return nullptr;
}

if (auto *begin = parser::cast_node<parser::Begin>(body.get())) {
// The body is already a Begin node, so we don't need any wrapping
walkNode(body.get());
Expand All @@ -324,7 +309,7 @@ unique_ptr<parser::Node> CommentsAssociator::walkBody(parser::Node *node, unique
// The body is a single node, we'll need to wrap it if we find standalone RBS comments
auto beforeNodes = parser::NodeVec();

// Visit standalone RBS comments after before the body node
// Visit standalone RBS comments before the body node
auto currentLine = core::Loc::pos2Detail(ctx.file.data(ctx), body->loc.beginPos()).line;
maybeInsertStandalonePlaceholders(beforeNodes, 0, lastLine, currentLine);
lastLine = currentLine;
Expand Down Expand Up @@ -393,6 +378,10 @@ void CommentsAssociator::walkNode(parser::Node *node) {
return;
}

// All nodes default to disallowing type aliases; Class/Module/SClass override this.
// Begin inherits from parent to be transparent.
contextAllowingTypeAlias.push_back(make_pair(false, node->loc));

typecase(
node,

Expand All @@ -418,6 +407,11 @@ void CommentsAssociator::walkNode(parser::Node *node) {
consumeCommentsInsideNode(node, "assign");
},
[&](parser::Begin *begin) {
// Begin is a container - inherit parent's type alias permission
if (contextAllowingTypeAlias.size() >= 2) {
contextAllowingTypeAlias.back() = contextAllowingTypeAlias[contextAllowingTypeAlias.size() - 2];
}

// This is a workaround that will be removed once we migrate to prism. We need to differentiate between
// implicit and explicit begin nodes.
//
Expand Down Expand Up @@ -473,14 +467,14 @@ void CommentsAssociator::walkNode(parser::Node *node) {
consumeCommentsInsideNode(node, "case");
},
[&](parser::Class *cls) {
contextAllowingTypeAlias.push_back(make_pair(true, cls->declLoc));
// Replace the default false with true - type aliases are allowed in class bodies
contextAllowingTypeAlias.back() = make_pair(true, cls->declLoc);
associateSignatureCommentsToNode(node);
auto beginLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.beginPos()).line;
consumeCommentsUntilLine(beginLine);
cls->body = walkBody(cls, move(cls->body));
auto endLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.endPos()).line;
consumeCommentsBetweenLines(beginLine, endLine, "class");
contextAllowingTypeAlias.pop_back();
},
[&](parser::CSend *csend) {
if (csend->method.isSetter(ctx.state) && csend->args.size() == 1) {
Expand All @@ -494,18 +488,14 @@ void CommentsAssociator::walkNode(parser::Node *node) {
consumeCommentsInsideNode(node, "csend");
},
[&](parser::DefMethod *def) {
contextAllowingTypeAlias.push_back(make_pair(false, def->declLoc));
associateSignatureCommentsToNode(node);
def->body = walkBody(def, move(def->body));
consumeCommentsInsideNode(node, "method");
contextAllowingTypeAlias.pop_back();
},
[&](parser::DefS *def) {
contextAllowingTypeAlias.push_back(make_pair(false, def->declLoc));
associateSignatureCommentsToNode(node);
def->body = walkBody(def, move(def->body));
consumeCommentsInsideNode(node, "method");
contextAllowingTypeAlias.pop_back();
},
[&](parser::Ensure *ensure) {
walkNode(ensure->body.get());
Expand Down Expand Up @@ -537,13 +527,15 @@ void CommentsAssociator::walkNode(parser::Node *node) {
walkNode(if_->condition.get());

lastLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.beginPos()).line;
auto *locationNode = if_->then_ ? if_->then_.get() : if_;
if_->then_ = walkBody(locationNode, move(if_->then_));

if (if_->then_) {
if_->then_ = walkBody(if_->then_.get(), move(if_->then_));
lastLine = core::Loc::pos2Detail(ctx.file.data(ctx), if_->then_->loc.endPos()).line;
} else if (if_->else_) {
// For `unless`, then_ is nil - advance lastLine so comments aren't grabbed early
lastLine = core::Loc::pos2Detail(ctx.file.data(ctx), if_->else_->loc.beginPos()).line;
}
locationNode = if_->else_ ? if_->else_.get() : if_;

auto *locationNode = if_->else_ ? if_->else_.get() : if_;
if_->else_ = walkBody(locationNode, move(if_->else_));

if (beginLine != endLine) {
Expand Down Expand Up @@ -574,14 +566,14 @@ void CommentsAssociator::walkNode(parser::Node *node) {
consumeCommentsInsideNode(node, "masgn");
},
[&](parser::Module *mod) {
contextAllowingTypeAlias.push_back(make_pair(true, mod->declLoc));
// Replace the default false with true - type aliases are allowed in module bodies
contextAllowingTypeAlias.back() = make_pair(true, mod->declLoc);
associateSignatureCommentsToNode(node);
auto beginLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.beginPos()).line;
consumeCommentsUntilLine(beginLine);
mod->body = walkBody(mod, move(mod->body));
auto endLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.endPos()).line;
consumeCommentsBetweenLines(beginLine, endLine, "module");
contextAllowingTypeAlias.pop_back();
},
[&](parser::Next *next) {
// Only associate comments if the last expression is on the same line as the next
Expand Down Expand Up @@ -653,14 +645,14 @@ void CommentsAssociator::walkNode(parser::Node *node) {
consumeCommentsInsideNode(node, "return");
},
[&](parser::SClass *sclass) {
contextAllowingTypeAlias.push_back(make_pair(true, sclass->declLoc));
// Replace the default false with true - type aliases are allowed in singleton class bodies
contextAllowingTypeAlias.back() = make_pair(true, sclass->declLoc);
associateSignatureCommentsToNode(node);
auto beginLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.beginPos()).line;
consumeCommentsUntilLine(beginLine);
sclass->body = walkBody(sclass, move(sclass->body));
auto endLine = core::Loc::pos2Detail(ctx.file.data(ctx), node->loc.endPos()).line;
consumeCommentsBetweenLines(beginLine, endLine, "sclass");
contextAllowingTypeAlias.pop_back();
},
[&](parser::Send *send) {
if (parser::MK::isVisibilitySend(send) || parser::MK::isAttrAccessorSend(send)) {
Expand Down Expand Up @@ -732,6 +724,8 @@ void CommentsAssociator::walkNode(parser::Node *node) {
associateAssertionCommentsToNode(node);
consumeCommentsInsideNode(node, "other");
});

contextAllowingTypeAlias.pop_back();
}

CommentMap CommentsAssociator::run(unique_ptr<parser::Node> &node) {
Expand All @@ -747,7 +741,10 @@ CommentMap CommentsAssociator::run(unique_ptr<parser::Node> &node) {
}

lastLine = 0;
// Allow type aliases at the top level
contextAllowingTypeAlias.push_back(make_pair(true, core::LocOffsets::none()));
walkNode(node.get());
contextAllowingTypeAlias.pop_back();

// Check for any remaining comments
for (const auto &[line, comment] : commentByLine) {
Expand Down
0 test/testdata/rbs/empty_if_else_module.rb → test/testdata/rbs/empty_toplevel_branches.rb
View file
Open in desktop
File renamed without changes.
66 changes: 66 additions & 0 deletions test/testdata/rbs/type_alias_nested_contexts.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# typed: true
# enable-experimental-rbs-comments: true

# Test type aliases in nested contexts
# Type aliases are ONLY allowed directly in class/module/sclass bodies,
# NOT inside if/unless/while/methods/etc.

# Type alias directly in module body - should work
module DirectModuleTypeAlias
#: type directType = String
end

# Type alias directly in class body - should work
class DirectClassTypeAlias
#: type directType = Integer
end

# Type alias inside `if` at top level - should error
if RUBY_VERSION >= "3.0"
#: type topLevelIfType = String
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Unexpected RBS type alias comment
end

# Module inside if - type alias in module body works
if RUBY_VERSION >= "3.0"
module VersionSpecificModule
#: type versionType = String
end
end

# Class inside unless - type alias in class body works
unless ENV["SKIP_FEATURE"]
class FeatureClass
#: type featureType = Integer
end
end

# Type alias inside `if` inside module - should error
module TypeAliasInIfInModule
if RUBY_VERSION >= "3.0"
#: type conditionalType = String
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Unexpected RBS type alias comment
else
#: type elseType = Integer
# ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Unexpected RBS type alias comment
end

# Type alias outside if but still in module body - should work
#: type moduleLevelType = Float
end

# Type alias inside method - should error
class MethodTypeAliasError
def bad_method
#: type badType = String
# ^^^^^^^^^^^^^^^^^^^^^^^^ error: Unexpected RBS type alias comment
end
end

# Type alias inside singleton method - should error
class SingletonMethodTypeAliasError
def self.bad_singleton_method
#: type singletonBadType = Integer
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: Unexpected RBS type alias comment
end
end
Loading