Annotations #977

lsk567 · 2022-02-24T02:47:56Z

This is a pull request tracking the progress of an annotation system in LF (#756).

This PR extends the syntax to support "attributes," which has the form @<attrName>(<paramName1> = <paramValue1>, <paramName2> = <paramValue2>, ...). <paramName> is optional, though @oowekyala pointed out that it would be good to have named parameters to conform with Java annotation grammar.

As a proof of concept, the label mechanism is implemented using an @label attribute.

Example: Label.lf

target C {
    threads: 1,
    keepalive: true
};

@label(value="This is a controller.")
reactor Controller {
    
    output out:bool;
    
    @label("startup reaction")
    reaction(startup) -> out {==}
}

@label("This is a vision system.")
reactor Vision {
    @label("A timer")
    timer t;
    
    input _in:bool;
    output out:bool;
    state ramp_exists:bool(false);
    
    @label(value="Action") 
    logical action act;
    
    @label(value="Processing visual input")
    reaction(_in) -> out, act {==}
}

reactor Door {
    input _in:bool;
    state door:bool;
    
    reaction(_in) {==}
}

@label("Top-level reactor")
main reactor {
    
    controller  = new Controller();
    vision      = new Vision();
    door        = new Door();
    
    controller.out -> vision._in;
    vision.out -> door._in;
}

TODOs:

Address a Duplicate AttrParm 'name' issue.
Support a clean way to incorporate DSLs, such as logic, into attributes based on the attribute name.

lsk567 · 2022-02-24T02:49:29Z

org.lflang.diagram/src/org/lflang/diagram/synthesis/LinguaFrancaSynthesis.xtend

+					if (list.length != 0)
+						commentText = list.get(0).getAttrParms.get(0).getValue.replaceAll("^\"|\"$", "")
+				}
+			}


Not sure how to resolve this code duplication... Xtend's support for polymorphism seems insufficient to avoid it.

org.lflang/src/org/lflang/LinguaFranca.xtext

org.lflang/src/org/lflang/validation/LFValidator.java

Add test

and empty param list

lhstrh · 2022-06-18T00:54:49Z

org.lflang/src/org/lflang/validation/LFValidator.java

@@ -55,6 +57,7 @@
 import org.eclipse.xtext.validation.Check;
 import org.eclipse.xtext.validation.CheckType;
 import org.eclipse.xtext.validation.ValidationMessageAcceptor;
+import org.jetbrains.annotations.NotNull;


I don't think we want to introduce this dependency. It will break things for Eclipse users.

I build the eclipse product, and it looks like Epoch isn't broken by this dependency.

But it also seems like this can be safely removed.

How did you build Epoch? In Eclipse?

The point is not that it would break Epoch, it would prevent developers from building Epoch in Eclipse. This is because Eclipse doesn't use Maven or Gradle but it's own build system that is severely deprived from otherwise generally accessible dependencies like this one. We've run into this issue several times before...

Okay, got it...

lhstrh · 2022-06-18T00:55:24Z

org.lflang/src/org/lflang/validation/LFValidator.java

+    /**
+     * Specification of the structure of an annotation.
+     */
+    static class AttributeSpec {


This seems like a good candidate to factor out into its own file.

Everything until line 1296 could be moved there.

Factoring out seems a bit tricky, since check() (L1229) calls error(), which is a protected method inherited from AbstractDeclarativeValidator. We could move the check functions back to the validator class, but that would sort of invalidate the refactoring because most of the code is still in the validator class.

Could pass in a lambda?

We can override the error function and just call super, this will make it visible

Works like magic. Thanks for the great advice.

org.lflang/src/org/lflang/validation/LFValidator.java

lhstrh

Looks good! There is some code in the validator that still needs some attention.

lhstrh · 2022-06-18T01:01:43Z

Shall we punt on the DSLs in the interest of getting this merged in soon? They should be a proper extension, anyway, so this can be addressed at a later time.

oowekyala · 2022-06-18T16:12:46Z

org.lflang/src/org/lflang/ASTUtils.java

    /**
     * Search for an `@label` annotation for a given reaction.
-     * 
+     *
     * @param n the reaction for which the label should be searched
     * @return The annotated string if an `@label` annotation was found. `null` otherwise.
     */
    public static String label(Reaction n) {


note that this overload could be removed if binary compatibility is not important, which seems likely.

Sorry, I didn't get this comment. What binary compatibility?

If someone has compiled a class that depends on this method and we remove it, their class will break. Minor releases generally preserve binary compatibility. I'm not sure whether this java API is publicly supported since you've started public releases... If the only ide extensions that use the java API are in this repo then we can most likely remove it (we're also pre-1.0.0)

…ram types.

lhstrh

Much better! Left some more comments/requests.

org.lflang/src/org/lflang/validation/LFValidator.java

lhstrh · 2022-06-23T17:33:31Z

org.lflang/src/org/lflang/validation/LFValidator.java

    private static String USERNAME_REGEX = "^[a-z_]([a-z0-9_-]{0,31}|[a-z0-9_-]{0,30}\\$)$";
+


Lines 1743:1758 belong in the AttributeSpec class.

org.lflang/src/org/lflang/validation/AttributeSpec.java

Co-authored-by: Marten Lohstroh <[email protected]>

oowekyala · 2022-06-23T19:45:00Z

org.lflang/src/org/lflang/validation/LFValidator.java

+     * so that it can be called from the AttributeSpec class.
+     */
+    @Override
+    public void error(java.lang.String message, 


Technically you can also leave it protected. Protected visibility is subclasses + current package, so it would be visible in AttributeSpec, which is in this package. This is a detail though

lsk567 · 2022-06-24T05:41:53Z

The current Attribute syntax generates 2 warnings from Antlr.

warning(200): //home/runner/work/lingua-franca/lingua-franca/org.lflang/../org.lflang/srcgen/org/lflang/parser/antlr/internal/InternalLF.g:5661:3: Decision can match input such as "RULE_ID '-' RULE_ID '@' {RULE_ID, 'physical'}" using multiple alternatives: 1, 2
As a result, alternative(s) 2 were disabled for that input

warning(200): //home/runner/work/lingua-franca/lingua-franca/org.lflang/../org.lflang.ide/srcgen/org/lflang/ide/contentassist/antlr/internal/InternalLF.g:13820:2: Decision can match input such as "RULE_ID '-' RULE_ID '@' {RULE_ID, 'physical'}" using multiple alternatives: 1, 2
As a result, alternative(s) 2 were disabled for that input

It seems that the leading @ sign in the Attribute syntax is confusing the parser since @ is also used in IPV4Host, IPV6Host, and NamedHost. Changing @ to @@ in the Attribute syntax got rid of the warnings, which confirms the previous observation, but we need a cleaner solution.

Any thoughts on this? @lhstrh @oowekyala

Soroosh129 · 2022-06-24T06:03:33Z

There are two possible solutions that come immediately to my mind:

Get rid of the at notation entirely and use attributes instead if needed
Change Host to just a string and check it in the validator

Under both solutions, IPV4Host, IPV6Host, and NamedHost are no longer needed.

lhstrh · 2022-06-24T07:26:45Z

Any thoughts on this? @lhstrh @oowekyala

The ambiguity comes from the "at" clause of an instantiation in combination with the annotations that can be featured inside of a reactor definition (i.e., above declarations of inputs, outputs, reactions, etc.).

Once the parser encounters something like x = new X() at foo followed by an @bar, its doesn't know whether the @bar should be part of the host or, instead, is an annotation of the subsequent declaration (such as a input y:baz).

oowekyala · 2022-06-24T07:58:18Z

@lhstrh is right. One solution would be to mandate a semicolon to terminate all declarations. I also like Soroush's idea to change Host back to a string, the productions are really complicated and anyway some checking is still done in the validator:

lingua-franca/org.lflang/src/org/lflang/LinguaFranca.xtext

Lines 432 to 446 in b24e5b5

    
           IPV6Addr: 
        
               // NOTE: This rule is too permissive by design.  
        
               // Further checking is done during validation.  
        
               // IPV6 with truncation. 
        
               '::' | ('::' (IPV6Seg (':'))* IPV6Seg) | ((IPV6Seg (':'|'::'))+ IPV6Seg?) |  
        
               // (Link-local IPv6 addresses with zone index) "fe80::7:8%1" 
        
               (ID '::' IPV6Seg (':' IPV6Seg)* '%' (INT | ID)+) | 
        
               // IPv4-mapped IPv6 addresses and IPv4-translated addresses 
        
               ('::' IPV4Addr) | ('::' ID ':' (INT ':')? IPV4Addr) | 
        
               // IPv4-Embedded IPv6 Address 
        
               (((IPV6Seg (':' IPV6Seg)* '::') | (IPV6Seg (':' IPV6Seg)*) ':') IPV4Addr) 
        
           ;

Soroosh129 · 2022-06-24T13:55:18Z

I personally think that an attribute instead of at is the more elegant solution. Where a federate resides I believe is an attribute anyway.

How about something like this:

@Host([email protected]:1000)
foo = new Foo($

and

@Host([email protected]:1000)
federated reactor {}

lhstrh · 2022-06-24T16:03:04Z

While I agree that using an annotation or surrounding the host with quotes might be better, I think that for this PR it's preferable to just require the semicolon in the offending production. I think we might want to more carefully evaluate the 'at' clause and its use before changing it. We can link this convo in a new issue.

lhstrh · 2022-06-24T16:05:31Z

I just want to emphasize that we don't need a semicolon on all declarations, just on instantiations that have an at clause. The impact of this change would be minimal.

Soroosh129 · 2022-06-24T16:14:48Z

just on instantiations that have an at clause.

That would be a bit weird.

I think we might want to more carefully evaluate the 'at' clause and its use before changing it.

I'm not proposing to change its meaning in this PR. Just how it can be declared. If we convert it to an attribute, then we can of course later still have a discussion about whether to keep it or change what it does.

Soroosh129 · 2022-06-24T17:06:00Z

org.lflang/src/org/lflang/ASTUtils.java

+     *
+     * @throws IllegalArgumentException If the node cannot have attributes
+     */
+    public static String findLabelAttribute(EObject node) {


Would it make sense if this was in a separate class like AttributeUtils? Same for other stuff here that relate to attributes.

Yes, I like this idea.

lhstrh · 2022-06-24T18:57:55Z

I just want to emphasize that we don't need a semicolon on all declarations, just on instantiations that have an at clause. The impact of this change would be minimal.

Concretely, my suggestion for a quick fix to the ambiguity problem is to replace the following line on LinguaFranca.xtext:233:

    ')' ('at' host=Host)? ';'?;

with

    ')' (('at' host=Host ';') | ';'?);

lsk567 · 2022-06-26T00:12:26Z

Looks like we are ready to go?

lhstrh

Looks good! Thanks for making all the requested changes :-)

lsk567 added 2 commits February 22, 2022 23:42

Extend syntax

8bb0e06

Extend syntax, update diagram synthesis, update validator

861ac63

lsk567 commented Feb 24, 2022

View reviewed changes

lsk567 requested a review from lhstrh February 24, 2022 02:49

Soroosh129 mentioned this pull request Mar 2, 2022

Outdated syntax for STA/STAA #975

Open

lsk567 commented Jun 8, 2022

View reviewed changes

org.lflang/src/org/lflang/LinguaFranca.xtext Outdated Show resolved Hide resolved

lsk567 commented Jun 8, 2022

View reviewed changes

org.lflang/src/org/lflang/LinguaFranca.xtext Outdated Show resolved Hide resolved

lsk567 commented Jun 8, 2022

View reviewed changes

org.lflang/src/org/lflang/LinguaFranca.xtext Show resolved Hide resolved

lsk567 commented Jun 8, 2022

View reviewed changes

org.lflang/src/org/lflang/validation/LFValidator.java Outdated Show resolved Hide resolved

oowekyala added 7 commits June 8, 2022 18:56

Merge branch 'master' into attribute

82fdef1

Add annotation validation

605c3f9

Support shorthand syntax

39f22de

Add test

add another test

f047cc8

Support trailing comma

9d15a71

and empty param list

Support attributes on more constructs

505efe6

Fix diagram synthesis

12ec7d5

lhstrh marked this pull request as ready for review June 18, 2022 00:26

lhstrh added 2 commits June 17, 2022 17:43

Resolve conflicts with master

bb74cf3

Fix remaining conflict

af7502e

lhstrh reviewed Jun 18, 2022

View reviewed changes

org.lflang/src/org/lflang/validation/LFValidator.java Outdated Show resolved Hide resolved

lhstrh requested changes Jun 18, 2022

View reviewed changes

oowekyala reviewed Jun 18, 2022

View reviewed changes

lsk567 added 4 commits June 22, 2022 09:19

Re-organize attributes in the LFValidator

bed6940

Add comments

080e5f8

Support basic param types such as String, Int, Boolean, and Float.

c30a7c9

Add a unit test that checks whether the validator can detect wrong pa…

14c7ec2

…ram types.

lsk567 requested a review from lhstrh June 23, 2022 05:19

lhstrh requested changes Jun 23, 2022

View reviewed changes

lsk567 and others added 2 commits June 23, 2022 11:32

Apply suggestions from code review

9b38e25

Co-authored-by: Marten Lohstroh <[email protected]>

Add author tags

b33484b

oowekyala reviewed Jun 23, 2022

View reviewed changes

lsk567 added 2 commits June 23, 2022 13:31

Move the static block and static map back to AttributeSpec

5de2169

Change the visibility of the overridden error() back to protected.

b24e5b5

lsk567 requested a review from lhstrh June 23, 2022 20:42

Soroosh129 reviewed Jun 24, 2022

View reviewed changes

lsk567 added 2 commits June 25, 2022 09:38

Apply quick fix suggested by @lhstrh to remove ambiguity

8b0d893

Factor out attribute-related methods into their own AttributeUtils class

c92516e

Soroosh129 mentioned this pull request Jun 26, 2022

Replace the at notation with attributes #1254

Open

lhstrh approved these changes Jun 27, 2022

View reviewed changes

lhstrh changed the title ~~Attributes~~ Attribute syntax Jun 27, 2022

lhstrh merged commit 292f812 into master Jun 27, 2022

lhstrh deleted the attribute branch June 27, 2022 19:23

petervdonovan added enhancement Enhancement of existing feature language Related to the syntax and semantics of LF labels Jul 1, 2022

lhstrh added feature New feature and removed enhancement Enhancement of existing feature labels Jul 7, 2022

lhstrh changed the title ~~Attribute syntax~~ Annotations Jul 20, 2022

		private static String USERNAME_REGEX = "^[a-z_]([a-z0-9_-]{0,31}\|[a-z0-9_-]{0,30}\\$)$";

Annotations #977

Annotations #977

Conversation

lsk567 commented Feb 24, 2022 • edited by lhstrh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsk567 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

lhstrh left a comment

Choose a reason for hiding this comment

lhstrh commented Jun 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhstrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsk567 commented Jun 24, 2022

Soroosh129 commented Jun 24, 2022 • edited Loading

lhstrh commented Jun 24, 2022

oowekyala commented Jun 24, 2022

Soroosh129 commented Jun 24, 2022

lhstrh commented Jun 24, 2022

lhstrh commented Jun 24, 2022

Soroosh129 commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhstrh commented Jun 24, 2022

lsk567 commented Jun 26, 2022

lhstrh left a comment

Choose a reason for hiding this comment

lsk567 commented Feb 24, 2022 •

edited by lhstrh

Loading

lsk567 Jun 23, 2022 •

edited

Loading

Soroosh129 commented Jun 24, 2022 •

edited

Loading