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

Jump to bottom

dyno: More progress on the typed converter #26824

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

dyno: More progress on the typed converter #26824

wants to merge 5 commits into from

Conversation

dlongnecke-cray
Copy link
Contributor

@dlongnecke-cray dlongnecke-cray commented Mar 4, 2025 •
edited
Loading

This PR adds more of my contributions to the typed converter. It adds support for compiling more language features such as parenless method calls, field access, logical operator short-circuiting, default values for records, and more! We can now write fun programs like the following.

record r {
  var x: int;
  proc method() { return x; }
}

var r1: r;
var x1 = r1.x;
var x2 = r1.method();
  • Support inserting AST at either the statement or expression level
  • Convert default-argument values for initializer calls (e.g., to support compiler-generated default-initializers)
  • Fold away param expressions immediately in convertExpr
  • Attach flags to generated AggregateType, mark them as RESOLVED
  • Convert opaque types::ExternType* and attach flags to them when the type variable for their declaration is evaluated
  • Construct default values for record types and extern records
  • Skip default-initialization under PRAGMA_NO_INIT
  • Convert tuple values e.g., (x, y)
  • Convert field access expressions with explicit receivers
  • Convert parenless method calls with explicit receivers
  • Convert || and && with support for short-circuiting
  • Generate default values for default-argument values in compiler-generated procedures
  • Convert dot expressions (e.g., x.y)
  • Add a method to BaseAST* called shouldNotMutateEarly()

TESTING

  • make -j && make check
  • make -j test-frontend
  • make -j test-cls
  • ALL w/ linux64, standard

Reviewed by @mppf. Thanks!

mppf
mppf approved these changes Mar 4, 2025
View reviewed changes
compiler/AST/view.cpp
@@ -658,13 +658,68 @@ void print_view_noline(BaseAST* ast) {
fflush(stdout);
}

// TODO: Take a reference and decay it to avoid copying when debugging.
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just a helper, how about making two different functions, one taking in a pointer, one taking in a reference? Might be possible for C++ overload resolution to handle this but I don't think fussing with that is worthwhile here. Just make two different functions with different names.

frontend/lib/parsing/parsing-queries.cpp
if (id.isEmpty() || !idIsMethod(context, id)) return false;
for (auto up = id.parentSymbolId(context); up;
up = up.parentSymbolId(context)) {
if (idIsMethod(context, up)) return true;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be checking for any sort of function here?

We already know Id is a method. I would think that this function should return true for a method nested in a non-method function.

Presumably search up should stop at a module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this function has a naming problem, what I want it to do is idIsMethodContainedInMethod. Should I rename it or just remove it from the frontend and move it to the converter? This is to catch the case of multiple conflicting receivers since we can't disambiguate those yet.

Will adjust to stop at module.

Copy link
Member

Choose a reason for hiding this comment

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

Renaming it to idIsMethodContainedInMethod is fine with me.

frontend/lib/types/CompositeType.cpp
if (this->hasPragma(context, uast::PRAGMA_EXTERN)) return uast::Decl::EXTERN;
if (this->hasPragma(context, uast::PRAGMA_EXPORT)) return uast::Decl::EXPORT;
if (auto& id = this->id()) {
if (auto ast = parsing::idToAst(context, id)) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should be storing the linkage in the CompositeType here rather than using idToAst. The CompositeType constructor / build can accept the linkage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

compiler/passes/convert-typed-uast.cpp
return ret;
}

std::tuple<AList*, Expr*, bool>& fetchListStackEntry(bool isStmtList) {
Copy link
Member

Choose a reason for hiding this comment

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

let's rename isStmtList to getStatement or something as it's not asserting about the list it's indicating what to get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about bool fetchStmtList? I think we have the same idea. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

compiler/passes/convert-typed-uast.cpp
@@ -2680,9 +3001,6 @@ VarSymbol* TConverter::convertVariable(const uast::Variable* node,
// Fix up the AST based on the type, if it should be known
setVariableType(node, varSym, rv);

// Note the variable is converted so we can wire up SymExprs later
noteConvertedSym(node, varSym);
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this ? Is it handled elsewhere now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm trying to be a bit more consistent with the naming scheme of convert... vs convertAndInsert... IDK. The visitor is inserting right now. Maybe it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, noteConvertedSym isn't about inserting it; it's about updating the maps from uAST things to AST things, so that if we see a mention of the variable later, we can use the dyno resolver to determine the ID it refers to, and then use the map to find the Symbol that is, and then generate a SymExpr pointing to the right thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mppf mppf mppf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dlongnecke-cray @mppf