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

feat(ast): add support for LambdaExpr to infer type from return expr type #1011

Merged
merged 5 commits into from
Jul 11, 2022

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Jul 6, 2022

This will enable it to be assigned to an appropriate variable

Before this change: cannot assign a lambda expression to a variable because its type is always VOID.
After this change: lambda expression builder will infer type from returnExpr’s expression type. Allowing it to be assigned to an appropriate variable. Because returnExpr is a required field, its type should always be accessible.
Example: a naive sample usage is provided in JavaWriterVisitorTest.java as test. Another example is what I am trying to generate for spring autoconfig code: this.projectIdProvider = () -> clientProperties.getProjectId();. Ideally through method reference, but lambda assignment does the same thing and it’s an easier feature addition.

@zhumin8 zhumin8 requested a review from blakeli0 July 6, 2022 20:54
@zhumin8 zhumin8 marked this pull request as ready for review July 6, 2022 20:54
@zhumin8 zhumin8 requested review from a team as code owners July 6, 2022 20:54
@blakeli0
Copy link
Collaborator

blakeli0 commented Jul 7, 2022

Looks a good feature improvement. If I'm understanding correctly, we cannot assign a lambda expression to a variable before this change because the return type is always void, so it would error out on checking the type of the left hand variable and the right hand lambda expression?
Can you please provide a little more description to the PR and maybe some examples?

@zhumin8
Copy link
Contributor Author

zhumin8 commented Jul 8, 2022

Yes, the improvement is exactly as you described.
I've updated the description above to include a bit more explanation.

// which would enable assignment to an appropriate variable.
return TypeNode.VOID;
}
public abstract TypeNode type();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping this despite sonar complains about already defined in Expr interface. It's seems the recommend way for @autovalue as explained in this doc. Also keeps consistent with other expression classes like ValueExpr, CastExpr

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might already considered it, can we just implement the type() method to return the type of ReturnExpr, like return returnExpr().expr().type()? So that we don't have to change any of the code in the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I actually did not think of that. Sounds like a better idea with less changes to existing code. The only thing is we won't be able to set a default value for type(), but that should be fine because ReturnExpr().expr() should always have type associated.
Let me make the changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want, we can still expose the setType() method and set a default value in the builder() method, but like you said ReturnExpr().expr() should always have type associated so we should never call setType() explicitly.

@sonarcloud
Copy link

sonarcloud bot commented Jul 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhumin8 zhumin8 merged commit a179558 into main Jul 11, 2022
@zhumin8 zhumin8 deleted the lambda_expr_infer_type branch July 11, 2022 17:46
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 14, 2022
🤖 I have created a release *beep* *boop*
---


## [2.9.0](v2.8.3...v2.9.0) (2022-07-11)


### Features

* **ast:** add support for LambdaExpr to infer type from return expr type ([#1011](#1011)) ([a179558](a179558))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Dec 16, 2022
…type (#1011)

This will enable it to be assigned to an appropriate variable

Before this change: cannot assign a lambda expression to a variable because its type is always VOID.

After this change: lambda expression builder will infer type from returnExpr’s expression type. Allowing it to be assigned to an appropriate variable. Because returnExpr is a required field, its type should always be accessible.

Example: a naive sample usage is provided in JavaWriterVisitorTest.java as test.
suztomo pushed a commit that referenced this pull request Dec 16, 2022
🤖 I have created a release *beep* *boop*
---


## [2.9.0](v2.8.3...v2.9.0) (2022-07-11)


### Features

* **ast:** add support for LambdaExpr to infer type from return expr type ([#1011](#1011)) ([b24892d](b24892d))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
…o v2.0.1 (#1011)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.api-client:google-api-client-bom](https://togithub.com/googleapis/google-api-java-client/tree/master/google-api-client-bom) ([source](https://togithub.com/googleapis/google-api-java-client)) | `2.0.0` -> `2.0.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.api-client:google-api-client-bom/2.0.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.api-client:google-api-client-bom/2.0.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.api-client:google-api-client-bom/2.0.1/compatibility-slim/2.0.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.api-client:google-api-client-bom/2.0.1/confidence-slim/2.0.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>googleapis/google-api-java-client</summary>

### [`v2.0.1`](https://togithub.com/googleapis/google-api-java-client/blob/HEAD/CHANGELOG.md#&#8203;201-httpsgithubcomgoogleapisgoogle-api-java-clientcomparev200v201-2022-11-05)

[Compare Source](https://togithub.com/googleapis/google-api-java-client/compare/v2.0.0...v2.0.1)

##### Bug Fixes

-   Add error description to batch emptiness validation ([#&#8203;2109](https://togithub.com/googleapis/google-api-java-client/issues/2109)) ([2668dd1](https://togithub.com/googleapis/google-api-java-client/commit/2668dd1348e7710a83e008b1e2b2ff6fceedfedf))
-   **deps:** Update dependency com.google.api-client:google-api-client to v2 ([#&#8203;2108](https://togithub.com/googleapis/google-api-java-client/issues/2108)) ([570a162](https://togithub.com/googleapis/google-api-java-client/commit/570a1625fbb3806961d328d90d784b5f0ed21a0c))
-   **deps:** Update dependency com.google.appengine:appengine-api-1.0-sdk to v2.0.10 ([#&#8203;2174](https://togithub.com/googleapis/google-api-java-client/issues/2174)) ([9077b4a](https://togithub.com/googleapis/google-api-java-client/commit/9077b4ae4c4214cb0fdcb5248f8c7ecbeb51d27f))
-   **deps:** Update dependency com.google.appengine:appengine-api-1.0-sdk to v2.0.6 ([#&#8203;2124](https://togithub.com/googleapis/google-api-java-client/issues/2124)) ([51adc54](https://togithub.com/googleapis/google-api-java-client/commit/51adc541819284dabcdefef39fa39b4a0bd13f6a))
-   **deps:** Update dependency com.google.appengine:appengine-api-1.0-sdk to v2.0.7 ([#&#8203;2131](https://togithub.com/googleapis/google-api-java-client/issues/2131)) ([6892bb2](https://togithub.com/googleapis/google-api-java-client/commit/6892bb293ca578b793fe0024c884b21e675abd45))
-   **deps:** Update dependency com.google.appengine:appengine-api-1.0-sdk to v2.0.8 ([#&#8203;2140](https://togithub.com/googleapis/google-api-java-client/issues/2140)) ([bb6f19c](https://togithub.com/googleapis/google-api-java-client/commit/bb6f19ce2a89f6d419c908eff7faa944ea74799e))
-   **deps:** Update dependency com.google.cloud:libraries-bom to v26.1.0 ([#&#8203;2126](https://togithub.com/googleapis/google-api-java-client/issues/2126)) ([3d0e0ff](https://togithub.com/googleapis/google-api-java-client/commit/3d0e0ff57cde5ca9eb56e5266dc5c37f3777179d))
-   **deps:** Update dependency com.google.cloud:libraries-bom to v26.1.1 ([#&#8203;2134](https://togithub.com/googleapis/google-api-java-client/issues/2134)) ([15ce062](https://togithub.com/googleapis/google-api-java-client/commit/15ce06244a06b64545f145a2ebdfb62863fcbad4))
-   **deps:** Update dependency com.google.cloud:libraries-bom to v26.1.2 ([#&#8203;2143](https://togithub.com/googleapis/google-api-java-client/issues/2143)) ([da2f6f3](https://togithub.com/googleapis/google-api-java-client/commit/da2f6f3e4645ff3b84465943833404526077ad20))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNy4xIiwidXBkYXRlZEluVmVyIjoiMzQuMTcuMSJ9-->
suztomo pushed a commit that referenced this pull request Mar 21, 2023
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

Successfully merging this pull request may close these issues.

2 participants