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.

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

Refactor helper functions around attributes #3291 #3293

Closed
wants to merge 2 commits into from
Closed

Refactor helper functions around attributes #3291 #3293

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfb6ef8
Refactor attribute checker logic into Attributes class
Harishankar14 Dec 5, 2024
06742a8
Merge branch 'master' into master
Harishankar14 Dec 5, 2024
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
193 changes: 129 additions & 64 deletions gcc/rust/util/rust-attributes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,18 @@ static const BuiltinAttrDefinition __definitions[]
{Attrs::RUSTC_CONST_UNSTABLE, STATIC_ANALYSIS},
{Attrs::PRELUDE_IMPORT, NAME_RESOLUTION}};

BuiltinAttributeMappings *
BuiltinAttributeMappings::get ()
{
static BuiltinAttributeMappings *instance = nullptr;
if (instance == nullptr)
instance = new BuiltinAttributeMappings ();
BuiltinAttributeMappings* BuiltinAttributeMappings::get() {
static std::mutex mtx;
static BuiltinAttributeMappings* instance = nullptr;

if (instance == nullptr) {
std::lock_guard<std::mutex> lock(mtx);
if (instance == nullptr) {
instance = new BuiltinAttributeMappings();
}
}
Comment on lines +80 to +88
Copy link
Member

Choose a reason for hiding this comment

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

why did you add a mutex here? we don't need one, as gccrs is single threaded and no two concurrent threads will try and access those attribute mappings.


return instance;
return instance;
}

const BuiltinAttrDefinition &
Expand All @@ -105,39 +109,77 @@ BuiltinAttributeMappings::BuiltinAttributeMappings ()
mappings.insert ({def.name, def});
}
}
class Attributes {
Copy link
Member

Choose a reason for hiding this comment

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

There is already an Attributes class in rust-attributes.h, which you should add functions to.

public:
// Check if an attribute with the specified name exists in the list
static bool has_attribute(const std::vector<Attribute> &attributes, const std::string &attr_name) {
return std::any_of(attributes.begin(), attributes.end(),
[&attr_name](const Attribute &attr) { return attr.name == attr_name; });
}

// Get the value of an attribute by its name, returning an empty string if not found
static std::string get_attribute_value(const std::vector<Attribute> &attributes, const std::string &attr_name) {
auto it = std::find_if(attributes.begin(), attributes.end(),
[&attr_name](const Attribute &attr) { return attr.name == attr_name; });
return (it != attributes.end()) ? it->value : ""; // Default or fallback if not found
}
static bool is_proc_macro_type(const AST::Attribute &attribute) {
BuiltinAttrDefinition result;
if (!is_builtin(attribute, result)) {
return false;
}

auto name = result.name;
return name == Attrs::PROC_MACRO || name == Attrs::PROC_MACRO_DERIVE
|| name == Attrs::PROC_MACRO_ATTRIBUTE;
}
};

AttributeChecker::AttributeChecker () {}

void
AttributeChecker::go (AST::Crate &crate)
void AttributeChecker::go(AST::Crate &crate)
{
visit (crate);
visit(crate);

// Use the centralized methods from the Attributes class
if (Attributes::has_attribute(crate.get_inner_attrs(), "example_attribute")) {
std::string value = Attributes::get_attribute_value(crate.get_inner_attrs(), "example_attribute");
// Now you can use the 'value' as needed
Comment on lines +144 to +147
Copy link
Member

Choose a reason for hiding this comment

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

Why are we checking if the crate has an "example attribute" attribute here? This seems like it shouldn't be here. The purpose of AttributeChecker::go is simply to provide a public entry-point for the visitor

}
}

void
AttributeChecker::visit (AST::Crate &crate)
{
check_attributes (crate.get_inner_attrs ());

for (auto &item : crate.items)
item->accept_vis (*this);
void AttributeChecker::visit(AST::Crate &crate)
{
check_attributes(crate.get_inner_attrs());

for (auto &item : crate.items) {
// Use the centralized methods from the Attributes class
if (Attributes::has_attribute(item->attributes, "example_attribute")) {
auto value = Attributes::get_attribute_value(item->attributes, "example_attribute");
// Now you can use the 'value' as needed
Comment on lines +157 to +160
Copy link
Member

Choose a reason for hiding this comment

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

Same here. We don't want to check for "example_attribute"

}
item->accept_vis(*this);
}
}


static bool
is_builtin (const AST::Attribute &attribute, BuiltinAttrDefinition &builtin)
{
auto &segments = attribute.get_path ().get_segments ();
// Retrieve path segments
const auto &segments = attribute.get_path().get_segments();

// Builtin attributes always have a single segment. This avoids us creating
// strings all over the place and performing a linear search in the builtins
// map
if (segments.size () != 1)
return false;
// Check if it's a single-segment path
Comment on lines -131 to +173
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove the comment?

if (segments.empty() || segments.size() != 1)
return false;

builtin = BuiltinAttributeMappings::get ()->lookup_builtin (
segments.at (0).get_segment_name ());
// Lookup the builtin attribute in mappings
builtin = BuiltinAttributeMappings::get()->lookup_builtin(
segments.at(0).get_segment_name());

return !builtin.is_error ();
// Return true if the attribute is valid and not an error
return !builtin.is_error();
}

/**
Expand Down Expand Up @@ -224,67 +266,90 @@ check_doc_attribute (const AST::Attribute &attribute)
}
}

static bool
is_proc_macro_type (const AST::Attribute &attribute)
static bool is_proc_macro_type(const AST::Attribute &attribute)
{
BuiltinAttrDefinition result;
if (!is_builtin (attribute, result))
return false;

auto name = result.name;
return name == Attrs::PROC_MACRO || name == Attrs::PROC_MACRO_DERIVE
|| name == Attrs::PROC_MACRO_ATTRIBUTE;
return Attributes::is_proc_macro_type(attribute);
}
Comment on lines +269 to 272
Copy link
Member

Choose a reason for hiding this comment

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

This is good but we'd want to remove the function all together and instead call Attributes::is_proc_macro_type throughout the file.



// Emit an error when one encountered attribute is either #[proc_macro],
// #[proc_macro_attribute] or #[proc_macro_derive]
static void
check_proc_macro_non_function (const AST::AttrVec &attributes)
static void check_proc_macro_non_function(const AST::AttrVec &attributes)
{
for (auto &attr : attributes)
for (auto &attr : attributes)
{
if (is_proc_macro_type (attr))
rust_error_at (
attr.get_locus (),
"the %<#[%s]%> attribute may only be used on bare functions",
attr.get_path ().get_segments ()[0].as_string ().c_str ());
if (Attributes::is_proc_macro_type(attr)) { // Use the centralized method
rust_error_at(
attr.get_locus(),
"the %<#[%s]%> attribute may only be used on bare functions",
attr.get_path().get_segments()[0].as_string().c_str());
}
}
}


// Emit an error when one attribute is either proc_macro, proc_macro_attribute
// or proc_macro_derive
static void
check_proc_macro_non_root (AST::AttrVec attributes, location_t loc)
static void check_proc_macro_non_root(AST::AttrVec attributes, location_t loc)
{
for (auto &attr : attributes)
for (auto &attr : attributes)
{
if (is_proc_macro_type (attr))
{
rust_error_at (
loc,
"functions tagged with %<#[%s]%> must currently "
"reside in the root of the crate",
attr.get_path ().get_segments ().at (0).as_string ().c_str ());
}
if (Attributes::is_proc_macro_type(attr)) { // Centralized check
rust_error_at(
loc,
"functions tagged with %<#[%s]%> must currently "
"reside in the root of the crate",
attr.get_path().get_segments().at(0).as_string().c_str());
}
}
}

enum class BuiltinAttributeType {
DOC,
PROC_MACRO,
PROC_MACRO_DERIVE,
PROC_MACRO_ATTRIBUTE,
UNKNOWN // Fallback for unrecognized attributes
};
BuiltinAttributeType Attributes::get_builtin_type(const std::string &name) {
static const std::unordered_map<std::string, BuiltinAttributeType> mapping = {
{Attrs::DOC, BuiltinAttributeType::DOC},
{Attrs::PROC_MACRO, BuiltinAttributeType::PROC_MACRO},
{Attrs::PROC_MACRO_DERIVE, BuiltinAttributeType::PROC_MACRO_DERIVE},
{Attrs::PROC_MACRO_ATTRIBUTE, BuiltinAttributeType::PROC_MACRO_ATTRIBUTE}
};

auto it = mapping.find(name);
return (it != mapping.end()) ? it->second : BuiltinAttributeType::UNKNOWN;
}

void
AttributeChecker::check_attribute (const AST::Attribute &attribute)
{
BuiltinAttrDefinition result;

// This checker does not check non-builtin attributes
if (!is_builtin (attribute, result))
return;

// TODO: Add checks here for each builtin attribute
// TODO: Have an enum of builtins as well, switching on strings is annoying
// and costly
if (result.name == Attrs::DOC)
check_doc_attribute (attribute);
BuiltinAttrDefinition result;

// Skip non-builtin attributes
if (!is_builtin(attribute, result))
return;

// Map attribute name to enum
BuiltinAttributeType type = Attributes::get_builtin_type(result.name);

// Perform specific checks based on type
switch (type) {
case BuiltinAttributeType::DOC:
check_doc_attribute(attribute);
break;

// Add more cases as needed for other types
case BuiltinAttributeType::UNKNOWN:
default:
// Handle unknown or unimplemented attribute types
break;
}
}


void
AttributeChecker::check_attributes (const AST::AttrVec &attributes)
{
Expand Down
Loading