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.

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

dyno: implement opaque extern types #22707

Merged
merged 7 commits into from
Jul 11, 2023
Merged

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Jul 10, 2023

This PR implements opaque extern types in Dyno. An opaque extern type is one defined as follows:

extern type myType;

Types like these are "opaque" in the sense that Chapel doesn't know their underlying representation; the name is pretty much the only identifying information about such types.

This PR diverges from production compiler behavior in that extern types with the same name are considered equal, even if they were declared in different places. In other words,

proc f1() type {
  extern type myType;
  return myType;
}

proc f2() type {
  extern type myType;
  return myType;
}

param x = f1() == f2(); // false in production, true with this PR

Reviewed by @benharsh -- thanks!

Testing

  • paratest

Copy link
Member

@benharsh benharsh left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I think we need to address the linkage-name vs. original-name question.

if (typeExpr == nullptr && initExpr == nullptr &&
var->linkage() == uast::Decl::EXTERN &&
var->storageKind() == QualifiedType::TYPE) {
debuggerBreakHere();
Copy link
Member

Choose a reason for hiding this comment

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

I think this is left over from development.


class ExternType final : public Type {
private:
UniqueString linkageName_;
Copy link
Member

Choose a reason for hiding this comment

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

I believe we need to preserve both the linkage name and the Chapel name. If a user writes:

extern "helper_t" type helper;
writeln(helper:string);

Then I believe it should print "helper" (and does in production, fwiw).

Copy link
Member

Choose a reason for hiding this comment

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

Adding a test that checks the stringify'd name of extern types would also be useful.

Copy link
Contributor Author

@DanilaFe DanilaFe Jul 10, 2023

Choose a reason for hiding this comment

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

Discussed this out of band, and Ben and I agreed that we can merge the PR as-is for the time being, and figure out how to keep the original name as part of the type later.

We don't want to show linkage names to the user instead of the Chapel type name, so this feature is important in the long term.

The reason this doesn't immediately work is that our type comparisons in canPass are based on pointer equality; however, if we were to store the ID or name of the variable that declared an opaque type in addition to the linkage name, two variables with distinct Chapel names but identical linkage names would not be the exact same type, and thus become incompatible. We don't want that. To address the issue, we probably want to do a deep comparison on types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also tracked here: #22711

@DanilaFe DanilaFe merged commit e2baf7b into chapel-lang:main Jul 11, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants