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

[Bug]: Inconsistent compile time behavior when deprecating record fields #40363

Closed
dilanSachi opened this issue May 8, 2023 · 6 comments · Fixed by #40545
Closed

[Bug]: Inconsistent compile time behavior when deprecating record fields #40363

dilanSachi opened this issue May 8, 2023 · 6 comments · Fixed by #40545
Assignees
Labels
Area/Compiler Reason/EngineeringMistake The issue occurred due to a mistake made in the past. Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Bug userCategory/Compilation
Milestone

Comments

@dilanSachi
Copy link
Contributor

Description

I need to deprecate a field in http:ListenerConfiguration.

public type ListenerConfiguration record {|
    # interceptors - An array of interceptor services
    # # Deprecated
    # Defining interceptor pipeline in `http:ListenerConfiguration` is deprecated. Define the interceptor pipeline
    # via `http:InterceptableService` service type
    @deprecated
    Interceptor|Interceptor[] interceptors?;
|};

This configuration is used in the http:Listener initialization as follows and accessed inside the init function.

public isolated function init(int port, *ListenerConfiguration config) returns ListenerError? {
   Interceptor|Interceptor[]? interceptors = config.interceptors;
}

Here, what we want is to show a deprecation notice to the users when the user tries to do the following.

http:Listener 'listener = check new (9090, interceptors = new RequestInterceptor());

This shows the deprecation warning when we build the http module (which is expected as we try to access the field). And when we build a user's module using the built http module, it doesnt show any warnings. Is this the expected behaviour? If so, how do we do something like that? (We had a case like this for http:ServiceConfig as well and used the compiler plugin to show a warning to the user.)

Apart from the above clarification question there's another weird behavior as well.

As mentioned, we get the compiler warning when building http module.

WARNING [ballerina/http/2.7.1::http_service_endpoint.bal:(55:55,55:74)] usage of construct 'ballerina/http:2.7.1:ListenerConfiguration.interceptors' is deprecated

Then we publish this module to local central and build a simple user's project.
Here also, we get the above warning. But this warning comes only when we do the project build for the first time. If we do a second bal build it is not present. To reproduce, we had to build, pack and push the module to the local central again.

Steps to Reproduce

Described above

Affected Version(s)

U5

OS, DB, other environment details and versions

No response

Related area

-> Compilation

Related issue(s) (optional)

ballerina-platform/ballerina-library#4300

Suggested label(s) (optional)

No response

Suggested assignee(s) (optional)

No response

@ballerina-bot ballerina-bot added needTriage The issue has to be inspected and labeled manually userCategory/Compilation labels May 8, 2023
@MaryamZi
Copy link
Member

MaryamZi commented May 9, 2023

Apart from the above clarification question there's another weird behavior as well.

As mentioned, we get the compiler warning when building http module.

WARNING [ballerina/http/2.7.1::http_service_endpoint.bal:(55:55,55:74)] usage of construct 'ballerina/http:2.7.1:ListenerConfiguration.interceptors' is deprecated

Then we publish this module to local central and build a simple user's project. Here also, we get the above warning. But this warning comes only when we do the project build for the first time. If we do a second bal build it is not present. To reproduce, we had to build, pack and push the module to the local central again.

This is because the HTTP module is built only on the first compilation. From the second compilation onward, the cached BIR will be used since available.

@MaryamZi MaryamZi added Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. and removed needTriage The issue has to be inspected and labeled manually labels May 9, 2023
@MaryamZi
Copy link
Member

MaryamZi commented May 9, 2023

Simplified sample

type Rec record {|
    @deprecated
    int[] x;
|};

public function main() {
    Rec r = {
        x: [] // no warning
    };
    r.x = []; // warning
    var x = r.x; // warning

    fn(x = []); // no warning
}

function fn(*Rec f) {
}

We don't seem to be logging warnings on initialization (extends to named args for fields of an included record param too).

The spec says warnings shouldn't be logged only when

a deprecated API element is used within another deprecated API element

@dulajdilshan
Copy link
Contributor

dulajdilshan commented May 23, 2023

We need to consider the following scenarios as well

type Rec record {
    @deprecated
    int[] x;
}

public function main() {
    // Scenario 1: variable reference as a field
    int[] x = [10];
    Rec t1 = {
         x
     };
    
    // Scenario 2: variable reference as a rest-field
    var val = { x : [1, 2]};
    Rec t2 = {
        ...val
    };
}

@dulajdilshan
Copy link
Contributor

function test() {
    name([]);
}

function name(@deprecated int[] x) {
}

The above example also doesn't give any warnings for deprecated usage.

@dulajdilshan
Copy link
Contributor

Have an Impact from #40902

@github-actions
Copy link

This issue is NOT closed with a proper Reason/ label. Make sure to add proper reason label before closing. Please add or leave a comment with the proper reason label now.

      - Reason/EngineeringMistake - The issue occurred due to a mistake made in the past.
      - Reason/Regression - The issue has introduced a regression.
      - Reason/MultipleComponentInteraction - Issue occured due to interactions in multiple components.
      - Reason/Complex - Issue occurred due to complex scenario.
      - Reason/Invalid - Issue is invalid.
      - Reason/Other - None of the above cases.

@MaryamZi MaryamZi added this to the 2201.8.0 milestone Aug 14, 2023
@dulajdilshan dulajdilshan added the Reason/EngineeringMistake The issue occurred due to a mistake made in the past. label Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area/Compiler Reason/EngineeringMistake The issue occurred due to a mistake made in the past. Team/CompilerFE All issues related to Language implementation and Compiler, this exclude run times. Type/Bug userCategory/Compilation
Projects
Archived in project
5 participants