Source section less node #35

jtulach · 2016-02-04T11:00:29Z

According to discussion in #25 it seems that there is a general consensus that SourceSections should be handled by individual languages and the interface in Truffle API simplified to:

Node.getSourceSection() that can be implemented by subclasses
RootNode(SourceSection, ...) constructor accepting SourceSection as majority of RootNode instances have source section

The other methods are deprecated while the functionality is kept. Once the deprecated methods are removed, the Node#sourceSection field will be removed as well. RootNode will then get such field to hold the value provided in constructor.

…ay we develop Truffle

…r subclasses to keep one and return it from the getSourceSection method

smarr · 2016-02-04T11:02:17Z

CONTRIBUTING.md

+When creating a pull request, make sure you designate an assignee to ensure
+the request is processed in a timely manner.
+
+[1]: http://www.oracle.com/technetwork/community/oca-486395.html


This looks like it should not be part of this pull request?

yes, please rebase.

…ncompatible

chumer · 2016-02-04T12:32:33Z

truffle/com.oracle.truffle.api/src/com/oracle/truffle/api/nodes/Node.java

+     * To create a node which can associate and change the {@link SourceSection} later at any point
+     * of time use:
+     *
+     * {@codesnippet source.section.mutable}
     *


I am still not convinced by the usefulness of these codesnippets. I would even say that most GL devs don't read javadocs but are browsing using the IDE where codesnippets dont translate nicely.

Hm, well, the issue would be little less problematic if at least the names would be proper JavaDoc names (like for @link) that one can click on to navigate to the example.

yes, including at least the classname would help a bit to find the snippet within an IDE...

I would prefer to leave the snippet discussion in #19 - I understand they are source of tension. That said the idea to make them behave like a link or at least include the name of source file is certainly worth exploring.

…e raw Javadoc to actual place in the code with the sample

woess · 2016-02-04T14:27:30Z

@jtulach I'd like to hear other people's opinion about this as well, but I think getSourceSection() should stay public.

mlvdv · 2016-02-04T19:38:36Z

getSourceSection() should be public. There will be tools (none yet) for which precise source attribution of nodes will be a requirement. This translates to the the invariant that a node was produced by parsing the text represented by its SourceSection. In contrast, getEncapsulatingSourceSection() should mean something else: what is the nearest enclosing syntactic unit that this Node is part of?
[Aside: getEncapsulatingSourceSection() only makes sense reliably when ASTs are "well-behaved" with respect to source attribution, something discussed in some detail elsewhere. Well-behaved ASTs are important for some kinds of tools, but this imposes a constraint on AST design that I've often seen violated, e.g. in javac.]
@mickjordan may want to comment; we spoke about this while the FastR AST was under development.

jtulach · 2016-02-05T03:55:27Z

I see. A real use-case for a tool that we yet have to imagine.

If I accept as given that getSourceSection is needed - do we need getEncapsulatingSourceSection() helper method at all? It seems to me like utility method that iterates over chain of nodes. Utility methods don't belong into core API. Each tool write the loop on their own.

ghost · 2016-02-05T06:20:18Z

On 2/4/16 11:38 AM, Michael Van De Vanter wrote:

|getSourceSection()| should be public. There will be tools (none yet)
for which precise source attribution of nodes will be a requirement.
This translates to the the invariant that a node was produced by
parsing the text represented by its |SourceSection|. In contrast,
|getEncapsulatingSourceSection()| should mean something else: what is
the nearest enclosing syntactic unit that this |Node| is part of?

[Aside: |getEncapsulatingSourceSection()| only makes sense reliably
when ASTs are "well-behaved" with respect to source attribution,
something discussed in some detail elsewhere. Well-behaved ASTs are
important for some kinds of tools, but this imposes a constraint on
AST design that I've often seen violated, e.g. in /javac/.]
@mickjordan https://github.com/mickjordan may want to comment; we
spoke about this while the FastR AST was under development.

In FastR we removed all uses of getEncapsulatingSourceSection and throw
an exception if anyone uses it. Our model is that you first find the
RSyntaxNode (getRSyntaxNode) and then call getSourceSection, which must
not fail. We actually try hard to avoid accessing the source section
until we really need it, like printing an error with context. We do
support a hierarchical search by default for getRSyntaxNode, but a node
can override if it's node structure is not hierarchical. As you recall I
tried to get this model into truffle without success, so we have a
special subclass of Node that contains the new methods and overrides of
the Truffle ones. It's a slight pain to have to remember to use this in
preference to Node. You can subclass Node directly but you have to
follow some rules to be compliant with the model.

Mick

jtulach · 2016-02-05T07:13:21Z

I see that FastR overrides getEncapsulatingSourceSetion to delegate to protected method that finds the right node and extracts the section from it. I see four overrides of the method. FastR overrides getSourceSection, but only to throw exceptions in unexpected cases. FastR calls getSourceSection - but mostly only because it passes the section somewhere else (assignSourceSection or constructor of other node).

As far as I can tell FastR would benefit from language controlled find/getSourceSection for a node.

chumer · 2016-02-09T10:01:00Z

I like the change. +1

I'd leave getSourceSection public for now, but maybe revisit this in another changeset? I think the instrumentation tools could use accessors to access the source section. Tools can access source sections using the EventContext via public API.

@chrisseaton @woess please also accept or deny this change

@jtulach After that please rebase and merge.

chrisseaton · 2016-02-09T12:41:36Z

truffle/com.oracle.truffle.api.dsl/src/com/oracle/truffle/api/dsl/internal/DSLShare.java

@@ -146,7 +146,8 @@ public T call() {

    private static void updateSourceSection(Node oldNode, Node newNode) {
        if (newNode.getSourceSection() == null) {
-            newNode.assignSourceSection(oldNode.getSourceSection());
+            throw new IllegalArgumentException("Can't update source: " + oldNode);
+            // newNode.assignSourceSection(oldNode.getSourceSection());


Comment left in by mistake?

I kept the comment as I expected the exception to be raised at some places, but it wasn't raised yet. But we haven't yet executed all our language tests. Maybe it is wiser to keep it for now.

chrisseaton · 2016-02-09T12:53:41Z

I'm not sure what the final proposed change is, as @chumer's last comments suggests more changes which may or may not be accepted, so it's hard to accept right now.

I currently use getEncapsulatingSourceSection on arbitrary foreign nodes to print backtraces for other language's stack frames in Ruby, so I'd need that functionality to remain or an alternative.

I don't mind maintaining the SourceSection field myself in Ruby, that seems like a good idea so languages can do it in different ways, but I think getSourceSection (maybe with the semantics of getEncapsulatingSourceSection) should stay, given the example of my backtraces.

woess · 2016-02-09T23:16:11Z

@chumer @jtulach
I accept the change (needs to be rebased though).
For now, getSourceSection(), getEncapsulatingSourceSection(), and RootNode should be kept as is, and any suggestion to change them is out of scope of this PR.

jtulach · 2016-02-10T13:18:26Z

OK, this change request will just deprecate Node(SourceSection) constructor and Node#assignSourceSection and Node#cleanSourceSection. Unless there are objections, let's merge tomorrow.

jtulach · 2016-02-11T07:36:30Z

No objections. Merging.

Deprecating Node(SourceSection) constructor and Node#assignSourceSection and Node#cleanSourceSection

…/truffle:DebuggerStatement to master DebuggerTags.AlwaysHalt * commit 'bfd6702c14dee81f9a189e6aad166ddce9eea462': AlwaysHalt tag introduced, implementation and test in SL.

Jaroslav Tulach added 2 commits February 1, 2016 13:19

Description of contributing procedure made more appropriate for the w…

0acb68c

…ay we develop Truffle

Deprecating Node constructor that accepts SourceSection leaving it fo…

c77efc1

…r subclasses to keep one and return it from the getSourceSection method

jtulach assigned chumer Feb 4, 2016

smarr reviewed Feb 4, 2016
View reviewed changes

Making getEncapsulatingSourceSection method final would be backward i…

00272a9

…ncompatible

chumer reviewed Feb 4, 2016
View reviewed changes

Giving codesnippet real class name to allow easier navigation from th…

5e7fcc9

…e raw Javadoc to actual place in the code with the sample

woess added the oracle-emp label Feb 4, 2016

chrisseaton reviewed Feb 9, 2016
View reviewed changes

Jaroslav Tulach added 2 commits February 10, 2016 13:21

Merging most recent changes in master branch in

4cdd994

Don't use deprecated constructor. Use the default one.

8443eab

jtulach mentioned this pull request Feb 10, 2016

SourceSection handling in Node #25

Closed

jtulach pushed a commit that referenced this pull request Feb 11, 2016

Merge pull request #35 from jtulach/SourceSectionLessNode

c688634

Deprecating Node(SourceSection) constructor and Node#assignSourceSection and Node#cleanSourceSection

jtulach merged commit c688634 into oracle:master Feb 11, 2016

jtulach deleted the SourceSectionLessNode branch February 11, 2016 07:36

jtulach restored the SourceSectionLessNode branch July 13, 2016 16:02

brunoborges unassigned chumer Jan 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source section less node #35

Source section less node #35

jtulach commented Feb 4, 2016

smarr Feb 4, 2016

woess Feb 9, 2016

chumer Feb 4, 2016

smarr Feb 4, 2016

woess Feb 4, 2016

jtulach Feb 4, 2016

woess commented Feb 4, 2016

mlvdv commented Feb 4, 2016

jtulach commented Feb 5, 2016

ghost commented Feb 5, 2016

jtulach commented Feb 5, 2016

chumer commented Feb 9, 2016

chrisseaton Feb 9, 2016

jtulach Feb 10, 2016

chrisseaton commented Feb 9, 2016

woess commented Feb 9, 2016

jtulach commented Feb 10, 2016

jtulach commented Feb 11, 2016

Source section less node #35

Source section less node #35

Conversation

jtulach commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woess commented Feb 4, 2016

mlvdv commented Feb 4, 2016

jtulach commented Feb 5, 2016

ghost commented Feb 5, 2016

jtulach commented Feb 5, 2016

chumer commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisseaton commented Feb 9, 2016

woess commented Feb 9, 2016

jtulach commented Feb 10, 2016

jtulach commented Feb 11, 2016