[analysis_server] Don't include expressions/getters in inline values for the current line

For convenience, we usually include Inline Values for the "current" line so that when stopped at an `if` statement, you can see their values:

```
if (foo == 1) {      // [foo = 2]
```

We have an experiment that includes inline values for getters (since although they can have side-effects, users tend to expect to see them in debug views, especially when the getter/setter are implicit), but triggering them before the code ever would have may be more unexpected.

This change uses two different ranges so that only variables are included for the current line.

Change-Id: I19a129a28f7b5296281dbee139855c738cd37194
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/418340
Commit-Queue: Phil Quitslund <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Phil Quitslund <[email protected]>

Author: DanTup

pkg/analysis_server/lib/src/lsp/handlers/handler_inline_value.dart

@@ -53,12 +53,19 @@ class InlineValueHandler
       }
       var lineInfo = unitResult.lineInfo;
 
-      // We will provide values from the start of the visible range up to
-      // the end of the line the debugger is stopped on (which will do by just
-      // jumping to position 0 of the next line).
+      // Compute the ranges for which we will provide values. We produce two
+      // ranges here because for some kinds of variables (simple values) it's
+      // convenient to see them on an `if` statement on the same line that
+      // hasn't executed yet. However, we should avoid evaluating getters which
+      // may have side effects if they haven't executed previously, because this
+      // may change state in a way that's less obvious.
       var visibleRange = params.range;
       var stoppedLocation = params.context.stoppedLocation;
-      var applicableRange = Range(
+      var rangeAlreadyExecuted = Range(
+        start: visibleRange.start,
+        end: stoppedLocation.end,
+      );
+      var rangeIncludingCurrentLine = Range(
         start: visibleRange.start,
         end: Position(line: stoppedLocation.end.line + 1, character: 0),
       );
@@ -75,7 +82,11 @@ class InlineValueHandler
           return success(null);
         }
 
-        var collector = _InlineValueCollector(lineInfo, applicableRange);
+        var collector = _InlineValueCollector(
+          lineInfo,
+          rangeAlreadyExecuted: rangeAlreadyExecuted,
+          rangeIncludingCurrentLine: rangeIncludingCurrentLine,
+        );
         var visitor = _InlineValueVisitor(
           server.lspClientConfiguration,
           collector,
@@ -113,17 +124,28 @@ class _InlineValueCollector {
   /// A map of elements and their inline value.
   final Map<Element2, InlineValue> values = {};
 
-  /// The range for which inline values should be retained.
+  /// The range for which simple inline values should be returned.
+  ///
+  /// This should be approximately the range of the visible code on screen up to
+  /// the point of execution and including the current line.
+  final Range rangeIncludingCurrentLine;
+
+  /// The range for which complex inline values (such as getters) should be
+  /// returned.
   ///
   /// This should be approximately the range of the visible code on screen up to
   /// the point of execution.
-  final Range applicableRange;
+  final Range rangeAlreadyExecuted;
 
   /// A [LineInfo] used to convert offsets to lines/columns for comparing to
   /// [applicableRange].
   final LineInfo lineInfo;
 
-  _InlineValueCollector(this.lineInfo, this.applicableRange);
+  _InlineValueCollector(
+    this.lineInfo, {
+    required this.rangeAlreadyExecuted,
+    required this.rangeIncludingCurrentLine,
+  });
 
   /// Records an expression inline value for [element] with [offset]/[length].
   ///
@@ -136,6 +158,11 @@ class _InlineValueCollector {
 
     var range = toRange(lineInfo, offset, length);
 
+    // Don't record anything outside of the visible range (excluding next line).
+    if (!range.intersects(rangeAlreadyExecuted)) {
+      return;
+    }
+
     var value = InlineValue.t1(
       InlineValueEvaluatableExpression(
         range: range,
@@ -158,6 +185,11 @@ class _InlineValueCollector {
 
     var range = toRange(lineInfo, offset, length);
 
+    // Don't record anything outside of the visible range (including next line).
+    if (!range.intersects(rangeIncludingCurrentLine)) {
+      return;
+    }
+
     var value = InlineValue.t3(
       InlineValueVariableLookup(
         caseSensitiveLookup: true,
@@ -183,11 +215,6 @@ class _InlineValueCollector {
   void _record(InlineValue value, Element2 element) {
     var range = _getRange(value);
 
-    // Never record anything outside of the visible range.
-    if (!range.intersects(applicableRange)) {
-      return;
-    }
-
     // We only want to show each variable once, so keep only the one furthest
     // into the source (closest to the execution pointer).
     if (values[element] case var existingValue?) {

pkg/analysis_server/test/lsp/inline_value_test.dart

@@ -193,6 +193,23 @@ void f(A /*[0*/a/*0]*/, int b) {
     await verify_values(code, ofType: InlineValueVariableLookup);
   }
 
+  /// Unlike variables, which we include for the line of the execution pointer
+  /// (to aid with reviewing conditional statements), getters are only evaluated
+  /// if they are before the execution pointer to reduce the chance of
+  /// triggering side-effects before the code would have.
+  Future<void> test_property_range_onlyBeforePointer() async {
+    experimentalInlineValuesProperties = true;
+
+    code = TestCode.parse(r'''
+void f(String /*[0*/s/*0]*/) {
+  ^if (s.isNotEmpty) {
+  }
+}
+''');
+
+    await verify_values(code, ofType: InlineValueVariableLookup);
+  }
+
   Future<void> test_property_setter() async {
     experimentalInlineValuesProperties = true;