Autodesk-AutoCAD / AutoLispExt

Visual Studio Code Extension for AutoCAD® AutoLISP
https://marketplace.visualstudio.com/items?itemName=Autodesk.autolispext
Apache License 2.0
83 stars 29 forks source link

Go To Defnitition inside if goes to wrong statement #88

Open hansWurst-creator opened 3 years ago

hansWurst-creator commented 3 years ago

Describe the bug Go To Defnitition inside if jumps to wrong statement

Steps to reproduce the behavior: Test with the following code: (defun foo () (setq a 1) (if T (progn (setq a 2) (print a) ) ) )

image

Expected behavior Courser jumps to (setq a 2)

hualin-wu-2000 commented 3 years ago

I think maybe we have some mis-understanding here, for these two(setq) line codes, because there is no local function variable definition like:

  (defun foo (/a)

)

So for this case the a is a "global" variable, then it goes to the first(setq) codes by intent as we define the behavior initially.

hansWurst-creator commented 3 years ago

So then Go to Definition will only be really helpful for functions and not variables? Because with local variables you always end up in the defun statment. For proper use with local variable the Go to Implementation would be the correct choice?

An explenation of how the feature works inside of autolisp would be great.

hualin-wu-2000 commented 3 years ago

We want to the F12 can support it for Global variables, and I think now it works well for the Local variables. I am trying understand what is the "correct" behavior for this global variables case mentioned above.

hansWurst-creator commented 3 years ago

So with localized variable this is the current implementation: image

This looks inconsistent with how local functions are handled: image

I was expecting a similar behavior with value variables. More like when was the last time something assigned to a certain variable not that much in what scope it lives in.

Well Lisp is quite a different programming language from statically typed languages. So it is not easy to bring over established principles to Lisp. I really appreciate the work though. Even the feature in its current state is a huge step forward.

JD-Howard commented 3 years ago

It kind of looks like globalized variables are just finding the first declaration within its active function scope. Which would make its behavior pretty consistent with a localized variables behavior.

For functions, the lower (b) had the context of a function. IE, preceded by a parenthesis. However, when it was next evaluated as an argument going into the defun function, it then started looking for localization of a potential variable. I don't necessarily perceive that to be a bad thing.

I did start making a "call map" that essentially bubbled up all the variables & functions to a root level parent context, but it started getting ugly real fast. Probably still the only way to capture globals properly, but I'll have to find a cleaner way to do it.

JD-Howard commented 3 years ago

@hansWurst-creator I will be revisiting GoTo with some new tooling created for F2 renaming. One important piece of context is that the "right click function documentation" from a previous release silently added the @Global comment identifier that is now being used in F2 renaming logic to flag functions/variable as exported. It makes those names across all document sources eligible for renaming. Likewise, this will be the "cross document" identifier that enables GoToDefinition for variables. Functions will probably still continue to act the same way, but with some added preference if it locates an @Global version.

Also, note that the new tooling (probably not in an official release yet) has a deep understanding of variable scope, so it doesn't perform those operations blindly.

now keep that in mind for the following questions about variable behavior...

Has Localization

Non-Localized but is NOT @Global flagged in any document

Non-Localized and DOES have @Global in SOME document somewhere

The option A's would be nice for reverse engineering the life of a variable, but other than that edge case, I don't think any of the A's are the really the best implementation. I think the B's will take a bit more work to implement, but probably are the right workflow. I assume C's aren't desirable based on what you were saying earlier. Is there an option D I am missing?