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

Fix NPE within annotation attachments #42762

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mindula
Copy link
Contributor

@mindula mindula commented May 16, 2024

Purpose

$tilte

Fixes #41038

Approach

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

Samples

Screenshot 2024-05-16 at 14 46 20

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

@dulajdilshan dulajdilshan self-requested a review May 16, 2024 09:17
@mindula mindula added the Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. label May 16, 2024
@@ -4683,10 +4683,12 @@ private void validateAnnotationAttachmentExpr(BLangAnnotationAttachment annAttac
}

if (annAttachmentNode.expr != null) {
data.commonAnalyzerData.withinAnnotationExpr = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data.commonAnalyzerData.withinAnnotationExpr = true;
boolean prevWithinAnnotationExpr = data.commonAnalyzerData.withinAnnotationExpr;
data.commonAnalyzerData.withinAnnotationExpr = true;

this.typeChecker.checkExpr(annAttachmentNode.expr, data.env,
referredAnnotType.tag ==
TypeTags.ARRAY ? ((BArrayType) referredAnnotType).eType : annotType, data.prevEnvs,
data.commonAnalyzerData);
data.commonAnalyzerData.withinAnnotationExpr = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the previous value of the withinAnnotationExpr in here.

@@ -3182,6 +3182,9 @@ public void visit(BLangSimpleVarRef varRefExpr, AnalyzerData data) {
// TODO: call to isInLocallyDefinedRecord() is a temporary fix done to disallow local var references in
// locally defined record type defs. This check should be removed once local var referencing is supported.
if (((symbol.tag & SymTag.VARIABLE) == SymTag.VARIABLE)) {
if (data.commonAnalyzerData.withinAnnotationExpr && varName.getValue().equals("self")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

use the constant value defined for self keyword.


public function prefetchBooks() {
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for objects and service type definitions as well.

@@ -3182,6 +3182,9 @@ public void visit(BLangSimpleVarRef varRefExpr, AnalyzerData data) {
// TODO: call to isInLocallyDefinedRecord() is a temporary fix done to disallow local var references in
// locally defined record type defs. This check should be removed once local var referencing is supported.
if (((symbol.tag & SymTag.VARIABLE) == SymTag.VARIABLE)) {
if (data.commonAnalyzerData.withinAnnotationExpr && varName.getValue().equals("self")) {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this is not correct. Before reaching this logic, we check for self within the main scope and set the symbol for self, which means we are allowing the self variable within that scope. This is not correct. At line 3171(lookupMainSpaceSymbolInPackage), when checking for the symbol within the scope, we should not see self for this annotation scope.
What we did here is set the symbol of the variable and then handle it as a special case.

@@ -7169,6 +7169,7 @@ public static class CommonAnalyzerData {
boolean breakToParallelQueryEnv = false;
int letCount = 0;
boolean nonErrorLoggingCheck = false;
boolean withinAnnotationExpr = false;
Copy link
Member

Choose a reason for hiding this comment

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

If we do not define self within the annotation scope, there is no need to define a special variable for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Getting NPE when passing an instance method to an annotation
3 participants