Make Cache.get() just return the value.

Change-Id: I816a1c28569011215ad6cba9a7559513a7900386
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/246880
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analyzer/lib/src/dart/analysis/byte_store.dart

@@ -50,7 +50,18 @@ class MemoryCachingByteStore implements ByteStore {
 
   @override
   Uint8List? get(String key) {
-    return _cache.get(key, () => _store.get(key));
+    final cached = _cache.get(key);
+    if (cached != null) {
+      return cached;
+    }
+
+    final fromStore = _store.get(key);
+    if (fromStore != null) {
+      _cache.put(key, fromStore);
+      return fromStore;
+    }
+
+    return null;
   }
 
   @override

pkg/analyzer/lib/src/dart/analysis/cache.dart

@@ -2,37 +2,33 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'package:meta/meta.dart';
+
 /// LRU cache of objects.
 class Cache<K, V> {
   final int _maxSizeBytes;
   final int Function(V) _meter;
 
-  final _map = <K, V>{};
+  @visibleForTesting
+  final map = <K, V>{};
   int _currentSizeBytes = 0;
 
   Cache(this._maxSizeBytes, this._meter);
 
-  V? get(K key, V? Function() getNotCached) {
-    V? value = _map.remove(key);
-    if (value == null) {
-      value = getNotCached();
-      if (value != null) {
-        _map[key] = value;
-        _currentSizeBytes += _meter(value);
-        _evict();
-      }
-    } else {
-      _map[key] = value;
+  V? get(K key) {
+    final value = map.remove(key);
+    if (value != null) {
+      map[key] = value;
     }
     return value;
   }
 
   void put(K key, V value) {
-    V? oldValue = _map[key];
+    V? oldValue = map[key];
     if (oldValue != null) {
       _currentSizeBytes -= _meter(oldValue);
     }
-    _map[key] = value;
+    map[key] = value;
     _currentSizeBytes += _meter(value);
     _evict();
   }
@@ -40,7 +36,7 @@ class Cache<K, V> {
   void _evict() {
     if (_currentSizeBytes > _maxSizeBytes) {
       var keysToRemove = <K>[];
-      for (var entry in _map.entries) {
+      for (var entry in map.entries) {
         keysToRemove.add(entry.key);
         _currentSizeBytes -= _meter(entry.value);
         if (_currentSizeBytes <= _maxSizeBytes) {
@@ -48,7 +44,7 @@ class Cache<K, V> {
         }
       }
       for (var key in keysToRemove) {
-        _map.remove(key);
+        map.remove(key);
       }
     }
   }

pkg/analyzer/lib/src/dart/micro/cider_byte_store.dart

@@ -54,7 +54,7 @@ class CiderCachedByteStore implements CiderByteStore {
 
   @override
   CacheData? get(String key, Uint8List signature) {
-    var entry = _cache.get(key, () => null);
+    final entry = _cache.get(key);
 
     if (entry != null &&
         const ListEquality<int>().equals(entry.signature, signature)) {

pkg/analyzer/test/src/dart/analysis/cache_test.dart

@@ -2,7 +2,10 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:typed_data';
+
 import 'package:analyzer/src/dart/analysis/cache.dart';
+import 'package:analyzer_utilities/testing/map_entry_matcher.dart';
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
@@ -12,52 +15,47 @@ main() {
   });
 }
 
-List<int> _b(int length) {
-  return List<int>.filled(length, 0);
+Uint8List _b(int length) {
+  return Uint8List(length);
 }
 
 @reflectiveTest
 class CacheTest {
-  test_get_notFound_evict() {
-    var cache = _newBytesCache(100);
+  test_get() {
+    final cache = _newBytesCache(100);
 
-    // Request '1'.  Nothing found.
-    expect(cache.get('1', _noBytes), isNull);
+    expect(cache.get('1'), isNull);
+    expect(cache.map.entries, isEmpty);
 
-    // Add enough data to the store to force an eviction.
-    cache.put('2', _b(40));
-    cache.put('3', _b(40));
-    cache.put('4', _b(40));
+    cache.put('1', _b(10));
+    expect(cache.map.entries, [
+      isMapEntry('1', hasLength(10)),
+    ]);
+
+    expect(cache.get('1'), hasLength(10));
   }
 
-  test_get_notFound_retry() {
-    var cache = _newBytesCache(100);
+  test_get_reorders() {
+    final cache = _newBytesCache(100);
 
-    // Request '1'.  Nothing found.
-    expect(cache.get('1', _noBytes), isNull);
+    cache.put('1', _b(1));
+    cache.put('2', _b(2));
+    cache.put('3', _b(3));
+    cache.put('4', _b(4));
+    expect(cache.map.entries, [
+      isMapEntry('1', hasLength(1)),
+      isMapEntry('2', hasLength(2)),
+      isMapEntry('3', hasLength(3)),
+      isMapEntry('4', hasLength(4)),
+    ]);
 
-    // Request '1' again.
-    // The previous `null` result should not have been cached.
-    expect(cache.get('1', () => _b(40)), isNotNull);
-  }
-
-  test_get_put_evict() {
-    var cache = _newBytesCache(100);
-
-    // Keys: [1, 2].
-    cache.put('1', _b(40));
-    cache.put('2', _b(50));
-
-    // Request '1', so now it is the most recently used.
-    // Keys: [2, 1].
-    cache.get('1', _noBytes);
-
-    // 40 + 50 + 30 > 100
-    // So, '2' is evicted.
-    cache.put('3', _b(30));
-    expect(cache.get('1', _noBytes), hasLength(40));
-    expect(cache.get('2', _noBytes), isNull);
-    expect(cache.get('3', _noBytes), hasLength(30));
+    expect(cache.get('2'), hasLength(2));
+    expect(cache.map.entries, [
+      isMapEntry('1', hasLength(1)),
+      isMapEntry('3', hasLength(3)),
+      isMapEntry('4', hasLength(4)),
+      isMapEntry('2', hasLength(2)),
+    ]);
   }
 
   test_put_evict_first() {
@@ -66,36 +64,39 @@ class CacheTest {
     // 40 + 50 < 100
     cache.put('1', _b(40));
     cache.put('2', _b(50));
-    expect(cache.get('1', _noBytes), hasLength(40));
-    expect(cache.get('2', _noBytes), hasLength(50));
+    expect(cache.map.entries, [
+      isMapEntry('1', hasLength(40)),
+      isMapEntry('2', hasLength(50)),
+    ]);
 
     // 40 + 50 + 30 > 100
     // So, '1' is evicted.
     cache.put('3', _b(30));
-    expect(cache.get('1', _noBytes), isNull);
-    expect(cache.get('2', _noBytes), hasLength(50));
-    expect(cache.get('3', _noBytes), hasLength(30));
+    expect(cache.map.entries, [
+      isMapEntry('2', hasLength(50)),
+      isMapEntry('3', hasLength(30)),
+    ]);
   }
 
-  test_put_evict_firstAndSecond() {
+  test_put_evict_firstSecond() {
     var cache = _newBytesCache(100);
 
     // 10 + 80 < 100
     cache.put('1', _b(10));
     cache.put('2', _b(80));
-    expect(cache.get('1', _noBytes), hasLength(10));
-    expect(cache.get('2', _noBytes), hasLength(80));
+    expect(cache.map.entries, [
+      isMapEntry('1', hasLength(10)),
+      isMapEntry('2', hasLength(80)),
+    ]);
 
     // 10 + 80 + 30 > 100
     // So, '1' and '2' are evicted.
     cache.put('3', _b(30));
-    expect(cache.get('1', _noBytes), isNull);
-    expect(cache.get('2', _noBytes), isNull);
-    expect(cache.get('3', _noBytes), hasLength(30));
+    expect(cache.map.entries, [
+      isMapEntry('3', hasLength(30)),
+    ]);
   }
 
-  Cache<String, List<int>> _newBytesCache(int maxSizeBytes) =>
-      Cache<String, List<int>>(maxSizeBytes, (bytes) => bytes.length);
-
-  static List<int>? _noBytes() => null;
+  Cache<String, Uint8List> _newBytesCache(int maxSizeBytes) =>
+      Cache<String, Uint8List>(maxSizeBytes, (bytes) => bytes.length);
 }

pkg/analyzer_utilities/lib/testing/map_entry_matcher.dart

@@ -0,0 +1,31 @@
+// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:test/test.dart';
+
+/// Matches a [MapEntry] with matching [MapEntry.key] and [MapEntry.value].
+MapEntryMatcher isMapEntry(key, value) => MapEntryMatcher(key, value);
+
+class MapEntryMatcher extends Matcher {
+  final Matcher keyMatcher;
+  final Matcher valueMatcher;
+
+  MapEntryMatcher(key, value)
+      : keyMatcher = wrapMatcher(key),
+        valueMatcher = wrapMatcher(value);
+
+  @override
+  Description describe(Description description) => description
+      .add('MapEntry(key: ')
+      .addDescriptionOf(keyMatcher)
+      .add(', value: ')
+      .addDescriptionOf(valueMatcher)
+      .add(')');
+
+  @override
+  bool matches(item, Map matchState) =>
+      item is MapEntry &&
+      keyMatcher.matches(item.key, {}) &&
+      valueMatcher.matches(item.value, {});
+}