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

Modify processing of method invocations w/ default arguments #1886

Merged
merged 2 commits into from Aug 15, 2019

Conversation

@jameskoch
Copy link

commented Jul 30, 2019

New contributor; apologies in advance. :)

Fixes #1883.

I think I've understood where the current impl goes wrong w/r/t methods w/ default values. I've described this in a detailed comment in TextDocumentOps.scala.

Detecting the generated code pattern in Scala 2.11 required a heuristic. I took the liberty of repurposing the single cross-compiling source file (AttachmentOps.scala) as a single injection point for future cross-compiling sources (so renamed to VersionSpecificOps.scala).

Actual fix resides in TextDocumentOps. It is slightly kludgy, but minimally disruptive to the surrounding code. Curious what others think.

@olafurpg
Copy link
Member

left a comment

Wow, fantastic work tracking down this bug! This bug has been bothering me for a while when doing goto definition in Metals. Thank you 🙏

The changes look good to me, only two minor comments

Modify processing of method invocations w/ default arguments.
On Scala 2.11 we use a heuristic to determine if we're inside
a compiler-generated Block. On later versions an attachment is
available.

@jameskoch jameskoch force-pushed the jameskoch:issue1883 branch from 1438861 to c150600 Aug 1, 2019

@jameskoch

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

Added the expect case. Notably we're still generating symbols in the doc for several of the compiler artifacts; not sure if anyone holds that as a blocker:

local0 => val local x$1: Int
Int => scala/Int#
local1 => val local x$2: Int
Int => scala/Int#
local2 => val local x$3: Int
Int => scala/Int#
local3 => val local x$1: Int
Int => scala/Int#
local4 => val local x$2: Int
Int => scala/Int#
local5 => val local x$3: Int
Int => scala/Int#
local6 => val local x$4: Int
Int => scala/Int#
local7 => val local x$5: Int
Int => scala/Int#
local8 => val local x$6: Int
Int => scala/Int#
local9 => val local Msg$1: Msg
Msg => example/NamedApplyBlockCaseClassConstruction.Msg#
local10 => val local x$1: String
String => java/lang/String#
local11 => val local x$2: String
String => java/lang/String#
local12 => val local x$3: String
String => scala/Predef.String#

@jameskoch

This comment has been minimized.

Copy link
Author

commented Aug 9, 2019

@olafurpg I finally got around to signing the CLA. Would love to get my two CRs reviewed if you can find the time, or point me to another maintainer.

PS - Where will you be working post-holiday?

@olafurpg
Copy link
Member

left a comment

This looks great! I'm really happy to see this bug fixed and I'm sure a lot of Metals users will also appreciate the improved code navigation in method with default arguments (which are everywhere).

I'm sorry for the slow review cycle, it normally doesn't take this long to get a PR reviewed and merged. I've been on an unusually long vacation this summer. We have other maintainers in Scalameta besides myself that can review and merge PRs but for changes to deeper internals like in this PR I feel most comfortable reviewing myself.

The CLA is happy. I pushed a commit fixing the formatting errors, will merge once CI is green.

PS - Where will you be working post-holiday?

I started working at Twitter on Scala developer tooling stuff, which includes Scalameta-related projects.

@olafurpg olafurpg merged commit 598edf1 into scalameta:master Aug 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.