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

Can't read TreeNode.location from patch files #31579

Open
sigmundch opened this issue Dec 7, 2017 · 8 comments
Open

Can't read TreeNode.location from patch files #31579

sigmundch opened this issue Dec 7, 2017 · 8 comments
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-dart2js customer-vm front-end-cleanup P3 A lower priority bug or feature request

Comments

@sigmundch
Copy link
Member

Many nodes contain a fileoffset but no file uri. TreeNode.location searches up the tree to find the first node with a file uri (typically a member, class, or library). How does this work when using patch files?
It appears as if we use the tree node from the original library for these elements, and the body and annotations from the patch file are attached.

This is causing problems in the lookup of the location of these nodes. The offset corresponds to the patch file, but we try to look for it in the original library source instead. As a result we commonly hit "out-of-range" errors when calling TreeNode.location.

Here is a repro:

You'll see this error:

The compiler crashed: RangeError (offset): Invalid value: Not in range 0..566, inclusive: 975
#0      RangeError.checkValueInInterval (dart:core/errors.dart:267)
#1      Source.getLocation (package:kernel/ast.dart:5693:16)
#2      Program.getLocation (package:kernel/ast.dart:5487:31)
#3      _getLocationInProgram (package:kernel/ast.dart:5808:20)
#4      Procedure._getLocationInEnclosingFile (package:kernel/ast.dart:1563:12)
#5      TreeNode._getLocationInEnclosingFile (package:kernel/ast.dart:169:20)
#6      TreeNode.location (package:kernel/ast.dart:165:12)
#7      computeSourceSpanFromTreeNode (sdk/pkg/compiler/lib/src/kernel/element_map.dart:491:18)
#8      KernelToElementMapBaseMixin.getConstantValue (sdk/pkg/compiler/lib/src/kernel/element_map_mixins.dart:330:9)
#9      KernelToElementMapBaseMixin.getMetadata.<anonymous closure> (pkg/compiler/lib/src/kernel/element_map_mixins.dart:343:20)
#10     List.forEach (dart:core-patch/dart:core/array.dart:79)
#11     KernelToElementMapBaseMixin.getMetadata (sdk/pkg/compiler/lib/src/kernel/element_map_mixins.dart:342:17)
...

The failure occurs when looking for the location of the @patch annotation in printToConsole.

@sigmundch sigmundch added area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-dart2js P1 A high priority bug; for example, a single project is unusable or has many test failures labels Dec 7, 2017
@peter-ahe-google peter-ahe-google added front-end-cleanup and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Dec 13, 2017
@peter-ahe-google
Copy link
Contributor

Siggi has a workaround for this, so I'm removing the P1 status. We probably want to extend the kernel format later, so I've marked this "front-end-cleanup".

whesse pushed a commit that referenced this issue Dec 13, 2017
We have a fileUri for fields and procedures and it was missing in constructors.

This is needed to be able to correctly store the patch URI in patched
constructors and to be able to workaround
#31579.
Change-Id: Ic80d3dc87450ada8b39b555e9b16e162d0e40b45
Reviewed-on: https://dart-review.googlesource.com/29003
Commit-Queue: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Reviewed-by: Kevin Millikin <kmillikin@google.com>
whesse pushed a commit that referenced this issue Dec 13, 2017
Temporarily use the patch URI for procedures and constructors and adjust
file-offset on class metadata to workaround issue #31579.

This change should be reverted when we have proper tracking of both origin and
patch URIs for each patched element.

Change-Id: I451a39b57cb121c2de3b1a324adc8cdbb5e8962c
Reviewed-on: https://dart-review.googlesource.com/29004
Commit-Queue: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Peter von der Ahé <ahe@google.com>
@jensjoha jensjoha added the P3 A lower priority bug or feature request label Jan 5, 2018
@ghost ghost unassigned peter-ahe-google Jun 9, 2021
dart-bot pushed a commit that referenced this issue Jun 9, 2021
TreeNode.location may return null "if the node is orphaned".
This happens for instance in cases where nodes can't use a
fileOffset due to patch files (see #31579).

This will in turn obscure other errors as the error reporting
code will crash trying to access members on null.

TEST=existing.

Bug: #31579
Change-Id: I85b720ecf2cb09e46cbd1296671be523567d5a83
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/202920
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
@ghost ghost added the customer-vm label Oct 11, 2021
@ghost
Copy link

ghost commented Oct 11, 2021

I'm repeatedly running into this issue (working with FfiNative, touching patch files) and the only workaround I have is to modify the transformations to remove the fileOffset from the affected nodes.

Given this issue is nearing four years old it would be nice to see if we can put this on some kind of plan to fix.

@mraleph
Copy link
Member

mraleph commented Oct 11, 2021

/cc @johnniwinther

@johnniwinther
Copy link
Member

It is a known deficiency of the AST that it cannot hold file offsets for different files within the same member. The remedy is to set the file offsets to TreeNode.noOffset or to that of the enclosing member (which is what CFE currently does for annotations on patched members).

@ghost
Copy link

ghost commented Oct 11, 2021

Do we by any chance have mechanisms for detecting when we're operating over the patch versions of a node in e.g. the transformations? (Something like TreeNode.isPatch()?)

@johnniwinther
Copy link
Member

We don't.

@johnniwinther
Copy link
Member

For class members you could use bool isPatch(Member member) => member.fileUri != member.enclosingClass!.fileUri;.

@johnniwinther
Copy link
Member

With https://dart-review.googlesource.com/c/sdk/+/311660 ConstantExpression now have a valid file offset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. customer-dart2js customer-vm front-end-cleanup P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

5 participants