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

Should type inclusion copy metadata? #1311

Open
MaryamZi opened this issue Jul 8, 2024 · 1 comment
Open

Should type inclusion copy metadata? #1311

MaryamZi opened this issue Jul 8, 2024 · 1 comment

Comments

@MaryamZi
Copy link
Member

MaryamZi commented Jul 8, 2024

Description:
Consider the following simple example for record type inclusion.

annotation Attribute on field;

type Parent record {
    @Attribute
    string parentAttr;
};

type Child record {
    *Parent;

    @Attribute
    string childAttr;
};

Right now, the jBallerina implementation does not copy metadata when copying the fields, but it probably should (there have been multiple requirements for this for library modules too).

Since we require the including record to explicitly define the field when the same field is defined in two included records (#696 (comment)), I believe we wouldn't run into conflicts.

annotation record {| string name; |} Attribute on field;

type Root record {
    @Attribute {name: "root"}
    string parentAttr;
};

type Parent record {
    @Attribute {name: "parent"}
    string parentAttr;
};

// Compile-time error anyway since `parentAttr` is not explicitly defined.
// Once fixed, metadata will be what is explicitly specified on the field defined here.
type Child record {
    *Root;
    *Parent;

    @Attribute {name: "child"}
    string childAttr;
};

Documentation

I believe this is generally straightforward, but BFM allows referring to "Ballerina-defined names from within documentation strings". When the included record comes from an imported module, there could be

  • qualified names referring to constructs from modules imported in the module in which the included type is defined, but not imported in the module in which the including type is defined
  • non-qualified names referring to constructs in the imported module

So just copying as is may not produce the expected results?

Annotations

  • For const annotations, I believe this should be straightforward.
  • But for this to work for non-const annotations, I believe we have to define and use them as closures, as we do with field defaults and copying closures.

https://ballerina.io/spec/lang/master/#record-type-inclusion

For default values, the closure rather than the expression is copied in.

We can then use the result in a spread field.

@jclark
Copy link
Collaborator

jclark commented Jul 9, 2024

I agree with your conclusions

  • metadata should be copied
  • non-const annotations should be treated as closures: scoping is always lexical, so expressions should be evaluated in the context in which they occur

The tricky bit is the occurrence of qualified names in the BFM. I would suggest that the abstract data model for the documentation string needs to include not just the string itself, but also the import context i.e. the imports declared at the top of the source file. Does that make sense?

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

No branches or pull requests

2 participants