feature: introduce the concept of lexical scope (interface LexicalScope)

[pvojtechovsky]

Implements scanner, which knows the named elements of actually scanned context. It is used by model validators of #2683 to detect conflicts.

It is ready for review

[surli]

@pvojtechovsky you leave in the main comment that it's ready for review, but apparently it's not anymore? What's missing?

[pvojtechovsky]

I WiP it because it needed rebase (there was 2 PRs here). Now it is ready for review.

[monperrus]

thanks for the big feature, we need a little bit of time to review it correctly :-) (doc, tests)

[monperrus]

Hi Pavel,

Just checked-out locally.

I have hard time to understand the intent and the design:

• what's the goal of NameScope?
• why do we have both NameScope and NameScope.Scanner as public? Could we have only one public class?
• NameScope depends on its subclass SimpleNameScope, which is kind of bad design.

Merry Christmas, --Martin

[pvojtechovsky]

Hi Martin,

I believe it is hard to understand. I do not like that too, but I do not have better idea and I do not have time to do it better.

What is the purpose? While scanning AST there is always available a queue of NameScopes, which can map any simple name to an element of AST, which maps to that name in current scanning scope.

So whole feature consists of Scanner, which constructs and keeps NameScopes depending on the scanned element and provides current NameScope in each scanning step.

See test which scans the Type Renata and which checks the NameScopes at time when literal "1" and "2" are scanned.

Merry Christmas to You too ;-)
Pavel

[seintur]

I suggest fixing the sentence to clarify what is going on. I do not understand what you mean here.

[pvojtechovsky]

I have hard time to understand the intent and the design

I just refactored it. There are two public classes now:

• NameScopeScanner - scanner which knows current NameScope
• NameScope - a mapping of simple name to named CtElement, which is imported/defined in actually scanned scope.
  Other classes are internal and represents different kinds of of NameScopes.

It is ready for review.

[monperrus]

I'm working on it

[monperrus]

I think I understand :-) You're introducing "lexical scope" as first class concept in Spoon (see https://en.wikipedia.org/wiki/Scope_(computer_science)#Lexical_scoping).

Pushed some changes accordingly (names + tests)

Is that correct?

Can we KISS this PR and merge AbstractNameScope and its subclasses?

[pvojtechovsky]

I do not think that collecting of all scopes makes sense.
Note that scopes are even changing during scanning. For example:

void draw() {
 //scope1
int a;
//scope2
int b;
//scope3
}

while scanning the statements of method draw, there is only one LexicalScope object whose content is different while scanning each comment.

In all my current use cases (handling of imports #2683) I am scanning AST and I need current lexical scope only.

If you see an use case behind I can live with that. I hope that amount of collected elements is not too big to consume much memory... :-)

[monperrus]

Thanks for the explanation. Fixed.

[pvojtechovsky]

Is that correct?

yes.

Can we KISS this PR and merge AbstractNameScope and its subclasses?

Note that subclasses are already internal. I need only the interfaces, so I am open to any changes concerning subclasses if you think the design is better then. Feel free to refactor it.

[monperrus]

KISSed it

[monperrus]

@pvojtechovsky WDYT?

[pvojtechovsky]

I am ok with this KISS. I like new LexicalScope term. It fits well.
I just suggest to rename LexicalScopeBuilder to LexicalScopeScanner, because main purpose of this class is still "to scan" and to provide the LexicalScope during that scanning process.

Why do you think that it is better design to let TypeNameScope extends NameScopeImpl, while NameScopeImpl contains a specific implementation which is not used in TypeNameScope? I like more the design with abstract class, which exactly express what is shared code used by all children.
But if you think it is easier to maintain I can live with that. They are internal classes and I do not expect that anybody will ever need to extend them. And yes, 2 classes to maintain is better then 3 classes :-)

[monperrus]

I just suggest to rename |LexicalScopeBuilder| to |LexicalScopeScanner|, because main purpose of this class is still "to scan" and to provide the LexicalScope during that scanning process.

Agree, done.

Why do you think that it is better design to let |TypeNameScope| extends |NameScopeImpl|

|TypeNameScope reuses the parent and scoped element (constructor)|

I like more the design with abstract class, which exactly express what is shared code used by all children.

Yes, but it's harder to understand. I tend to prefer maintainability over pure, yet potentially over-engineered, design esp. for open-source.

But if you think it is easier to maintain I can live with that.

Perfect. @nharrand do you merge?

[nharrand]

Awesome feature. It would have saved me a lot of trouble a couple months ago!