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 Scala inference not inferring a symbol name after a function call #15565

Merged
merged 2 commits into from
May 23, 2022

Conversation

somdoron
Copy link
Contributor

@somdoron somdoron commented May 20, 2022

This fix solved most of our compilation errors without adding explicit dependencies.

The problem is if the qual part of the Select term is Apply term the ScalaParser doesn't build the full name, only the outer term name and the dependency cannot be inferred automatically. This PR continues and visits the qual node of the select term.

@Eric-Arellano Eric-Arellano requested review from tdyas and stuhood May 20, 2022 18:51
@Eric-Arellano Eric-Arellano added the category:bugfix Bug fixes for released features label May 20, 2022
@Eric-Arellano
Copy link
Contributor

Awesome! Thank you so much. I've pinged more qualified reviewers and kicked off CI

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fix. Will wait for @tdyas to take a look.

To fix the lint errors, you can run ./pants fmt src/python/pants/backend/scala/dependency_inference/scala_parser_test.py

Comment on lines 277 to 278
# Select with Apply as Qualifier only yield the name after the Apply
".toInt",
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bug... is it avoidable by recursing on a different portion of node.qual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is actually not related to the qual, it existed even before my pr, that the behavior of extractName when the qual is not one of the cases in the match, but let me try and fix that in another commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stuhood I've pushed a new commit that fixes the leading comma.

9f5782b

Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@Eric-Arellano
Copy link
Contributor

Yay! @tdyas @stuhood should we cherry-pick to 2.12 or 2.11?

@stuhood
Copy link
Member

stuhood commented May 23, 2022

Yay! @tdyas @stuhood should we cherry-pick to 2.12 or 2.11?

Definitely 2.12: I don't think that it needs to go back to 2.11 though, unless @somdoron needs it there. This is our first report of this case.

@stuhood stuhood changed the title fix scala inference doesn't infer name after function call Fix Scala inference not inferring a symbol name after a function call May 23, 2022
@stuhood stuhood merged commit f755c9b into pantsbuild:main May 23, 2022
@stuhood stuhood modified the milestones: 2.11.x, 2.12.x May 23, 2022
@somdoron
Copy link
Contributor Author

We are currently running pants from source using our branch because we need #15583. So we made a temporary patch to the ScalaParser:

somdoron@8569427

Bottom line, cherry-picking for 2.11 is not required for us.

stuhood pushed a commit to stuhood/pants that referenced this pull request May 23, 2022
…d#15565)

This fix solved most of our compilation errors without adding explicit dependencies.

The problem is if the `qual` part of the Select term is Apply term the ScalaParser doesn't build the full name, only the outer term name and the dependency cannot be inferred automatically. This PR continues and visits the `qual` node of the select term.

[ci skip-rust]
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request May 23, 2022
… (Cherry-pick of #15565) (#15603)

The problem is if the `qual` part of the Select term is Apply term the ScalaParser doesn't build the full name, only the outer term name and the dependency cannot be inferred automatically. This PR continues and visits the `qual` node of the select term.

[ci skip-rust]
[ci skip-build-wheels]

Co-authored-by: Doron Somech <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants