Skip to content

Fix namebinding#220

Merged
ltratt merged 2 commits into
masterfrom
fix_namebinding
Sep 20, 2018
Merged

Fix namebinding#220
ltratt merged 2 commits into
masterfrom
fix_namebinding

Conversation

@ptersilie

Copy link
Copy Markdown
Member

This PR fixes and improves namebinding across languages, which before needed some language specific hacks to work but now is a bit more language agnostic. It also fixes code completion which broke some time ago.

@ptersilie

Copy link
Copy Markdown
Member Author

The last commit probably needs squashing.

@ltratt

ltratt commented Sep 19, 2018

Copy link
Copy Markdown
Member

Do you want to do that before I review?

@ptersilie

Copy link
Copy Markdown
Member Author

Squashed!

Comment thread lib/eco/astanalyser.py Outdated
def convert_uri(self, newkind, prev, path):
uri = prev
if len(prev.path) >= 1:
# The top-level in all of our namebinding rules doesn't have a name, so

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First sentence is a bit hard to parse. Rephrase?

Comment thread lib/eco/astanalyser.py Outdated
# this is a semi safe way to find the top-level and replace it if there
# is one. Alternatively, we could force all namebinding rules to use the
# same name, which seems more restrictive.
# XXX This doesn't work if a namebinding rule gives the top-level a name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to contradict the comment's first sentence. Can we guard against this, or is it impossible, or ...?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't need to guard against it, as it doesn't result in an error or something. It just means that the namebinding rules from a language box won't be integrated into the outer language. If we had a documentation we would just say that top-level rules can't have a name. If we want to fix this properly, we have to somehow mark the top-level rule so we can check properly here if a given rule is the top-level or not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we guard against it so that if someone does specify this, at least they get an error, rather than things just silently not working?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let me have a look.

@ltratt

ltratt commented Sep 20, 2018

Copy link
Copy Markdown
Member

OK, please squash.

@ltratt

ltratt commented Sep 20, 2018

Copy link
Copy Markdown
Member

Ah, let's merge #221 first, then you can squash and rebase against master at the same time.

@ptersilie

Copy link
Copy Markdown
Member Author

Squashed!

Previously to enable cross-language scoping, we converted scoping types
from one language to another. For example, when embedding a Python method
into PHP, we renamed it to `function` so PHP's scoping rules could
reference it. This was restricitive as not every scope type has a true
representation in the other language. For example, in Python `method` can
be referenced by `variables`, which is not possible in PHP. Thus converting
`method` to `function` would disallow `method` to be referenced by a PHP
`variable`.

This commit changes how we treat namebinding information from language
boxes. When embedding Python into PHP, for example, Python's scoping rules
are merged into PHP's as is, keeping their specific types. We then extend
PHP's scoping rules, allowing them to reference Python types. For example,
a PHP `FunctionCall` now references `function` (PHP) and `method` (Python).
@ltratt ltratt merged commit a1618b6 into master Sep 20, 2018
@ltratt ltratt deleted the fix_namebinding branch September 20, 2018 16:54
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