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

Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] #23473

Merged
merged 10 commits into from
Jun 17, 2020
Merged

Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] #23473

merged 10 commits into from
Jun 17, 2020

Conversation

KRVPerera
Copy link
Contributor

@KRVPerera KRVPerera commented May 25, 2020 •
edited
Loading

Purpose

Fixes #15533

Approach

Describe how you are implementing the solutions along with the design details.

Samples

Provide high-level details about the samples related to this feature.

Remarks

List any other known issues, related PRs, TODO items, or any other notes related to the PR.

Check List

  • Read the Contributing Guide
  • Updated Change Log
  • Checked Tooling Support (#)
  • Added necessary tests
    • Unit Tests
    • Spec Conformance Tests
    • Integration Tests
    • Ballerina By Example Tests
  • Increased Test Coverage
  • Added necessary documentation
    • API documentation
    • Module documentation in Module.md files
    • Ballerina By Examples

@KRVPerera KRVPerera changed the title Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] [WIP] Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] May 25, 2020
@pubudu91 pubudu91 added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label May 25, 2020
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 6 times, most recently from 59a927a to 5ed037f Compare June 2, 2020 07:14
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 3 times, most recently from c63bb6c to 5a22d1c Compare June 8, 2020 17:15
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 2 times, most recently from 6cba4c0 to 940baac Compare June 11, 2020 09:58
@KRVPerera KRVPerera mentioned this pull request Jun 11, 2020
Closed
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 2 times, most recently from 0491081 to c3904f5 Compare June 12, 2020 08:58
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 3 times, most recently from 8d15074 to 3172e25 Compare June 14, 2020 09:47
@KRVPerera KRVPerera closed this Jun 14, 2020
@KRVPerera KRVPerera reopened this Jun 14, 2020
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch from 22b0b95 to f0b55bc Compare June 14, 2020 16:04
@KRVPerera KRVPerera changed the title [WIP] Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] Restrict annotation value to true, map<anydata|readonly> , map<anydata|readonly>[] Jun 14, 2020
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 2 times, most recently from 750b4a6 to 426ca09 Compare June 15, 2020 02:56
aashikam
aashikam previously approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@aashikam aashikam left a comment

Choose a reason for hiding this comment

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

RabbitMQ changes LGTM.

@KRVPerera KRVPerera marked this pull request as ready for review June 15, 2020 04:51
MaryamZi
MaryamZi reviewed Jun 16, 2020
View reviewed changes
...erina-lang/src/main/java/org/wso2/ballerinalang/compiler/semantics/analyzer/SymbolEnter.java
return types.isAssignable(type, symTable.trueType);
}

private boolean isAnyDataOrReadOnlyTypeSkippingObjectType(BType type) {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we create an issue to track fixing std lib deviations and add back the object validation to annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue #24217

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue to do language changes after StandardLib changes: #24246

@shafreenAnfar shafreenAnfar reopened this Jun 16, 2020
@shafreenAnfar
Copy link
Contributor

Closed and re-opened because the github workflow has been updated.

@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch from 0d31fb6 to 16a41d8 Compare June 16, 2020 12:04
@KRVPerera KRVPerera closed this Jun 16, 2020
@KRVPerera KRVPerera reopened this Jun 16, 2020
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch 4 times, most recently from d3941fa to 41858ae Compare June 16, 2020 22:30
@KRVPerera
Restrict annotation value type
57d457d 
Annotation value is restricted to true, map<anydata|readonly> or
map<anydata|readonly>[] value. Changes will also make sure that
annotation value is a read only clone of the given value. Therefore user
 cannot change annotation value via annotation access.
@KRVPerera
Fix grpc tests for annotation value restriction
b43010f
@KRVPerera
Fix rabbitmq tests : annotation value restriction
c3a2b4d
@KRVPerera
Disable language completion tests
dc0845b
@KRVPerera
Fix Code review suggestions
a77cbf6 
Uniformity for `readonly & T` and `T & readonly`. Chose `readonly & T`.
Refactor java code logic. Instead of making fields readonly for
implementations of abstract objects, whole object is made readonly.
@KRVPerera
Fix newly added testcases for correct AST
9bc0ad8
@KRVPerera
Restrict annotation only for cloneReadOnly wrap
dac5286
@KRVPerera
Fix a bug accepting true[] for annotation value
3e8b157
@KRVPerera
Support cloneReadOnly for immutable types
93e74ca
@KRVPerera
Remove object readonly check for annotation values
5342dcd
@KRVPerera KRVPerera force-pushed the krv_annotation_value_restriction branch from 3751e75 to 5342dcd Compare June 17, 2020 04:07
MaryamZi
MaryamZi reviewed Jun 17, 2020
View reviewed changes
bvm/ballerina-runtime/src/main/java/org/ballerinalang/jvm/values/TypedescValueImpl.java
@@ -107,6 +107,11 @@ public Object copy(Map<Object, Object> refs) {
return this;
}

@Override
public void freezeDirect() {
Copy link
Member

Choose a reason for hiding this comment

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

Please create an issue to track verifying these changes with @irshadnilam's changes.

...lerina-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationRuntimeTest.java
}

@Test(expectedExceptions = BLangRuntimeException.class,
expectedExceptionsMessageRegExp = "^error: \\{.*\\}InvalidUpdate message=Invalid update " +
Copy link
Member

Choose a reason for hiding this comment

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

Please create an issue for this too?

...lerina-unit-test/src/test/java/org/ballerinalang/test/annotations/AnnotationRuntimeTest.java
Comment on lines +72 to +73
resultAccessNegative = BCompileUtil.compile("test-src/annotations/annotation_access_negative" +
".bal");
Copy link
Member

Choose a reason for hiding this comment

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

Can go on one line.

MaryamZi
MaryamZi reviewed Jun 17, 2020
View reviewed changes
Copy link
Member

@MaryamZi MaryamZi left a comment

Choose a reason for hiding this comment

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

Can we add an annotation test where the annotation fields are the newly allowed readonly types?

A simple test for an annotation with object, service, function, handle, and typedesc fields.

@MaryamZi MaryamZi merged commit 457db3b into ballerina-platform:master Jun 17, 2020
@KRVPerera KRVPerera mentioned this pull request Jun 17, 2020
Closed
4 tasks
@KRVPerera
Copy link
Contributor Author

Created an issue to incorporated all feedbacks related to unit test cases.
#24245

@KRVPerera KRVPerera mentioned this pull request Jun 17, 2020
Merged
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaryamZi MaryamZi MaryamZi approved these changes

@daneshk daneshk daneshk left review comments

@aashikam aashikam aashikam left review comments

Assignees
No one assigned
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Annotations] Restrict constraints of maps/fields of records to be anydata and freeze the values
6 participants
@KRVPerera @shafreenAnfar @MaryamZi @daneshk @aashikam @pubudu91