From 93dc547d9d84607e0e4c35f018d4241e6a85d710 Mon Sep 17 00:00:00 2001 From: Devon Carew Date: Mon, 11 Dec 2023 10:59:24 -0800 Subject: [PATCH] allow analysis of snippets irregardless of imports (#2744) --- pkgs/dart_services/lib/src/analysis.dart | 91 +++++++++++-------- pkgs/dart_services/lib/src/pub.dart | 15 ++- pkgs/dart_services/lib/src/utils.dart | 34 +++++++ pkgs/dart_services/test/pub_test.dart | 53 ----------- .../{server_v3_test.dart => server_test.dart} | 18 +++- pkgs/dart_services/test/utils_test.dart | 38 ++++++++ 6 files changed, 147 insertions(+), 102 deletions(-) rename pkgs/dart_services/test/{server_v3_test.dart => server_test.dart} (94%) diff --git a/pkgs/dart_services/lib/src/analysis.dart b/pkgs/dart_services/lib/src/analysis.dart index 445aa3917..7d4803e27 100644 --- a/pkgs/dart_services/lib/src/analysis.dart +++ b/pkgs/dart_services/lib/src/analysis.dart @@ -6,18 +6,16 @@ import 'dart:async'; import 'dart:io'; import 'package:analysis_server_lib/analysis_server_lib.dart'; -import 'package:analyzer/dart/ast/ast.dart'; import 'package:logging/logging.dart'; import 'package:path/path.dart' as path; import 'common.dart'; -import 'common_server.dart'; -import 'project_templates.dart' as project; import 'project_templates.dart'; import 'pub.dart'; import 'sdk.dart'; import 'shared/model.dart' as api; import 'utils.dart' as utils; +import 'utils.dart'; final Logger _logger = Logger('analysis_server'); @@ -41,24 +39,14 @@ class Analyzer { } Future analyze(String source) async { - final imports = getAllImportsFor(source); - - // TODO(srawlins): Do the work so that each unsupported input is its own - // error with a proper SourceSpan. - await _checkPackageReferences(imports); - - return analysisServer.analyze(source, imports: imports); + return analysisServer.analyze(source); } Future complete(String source, int offset) async { - await _checkPackageReferences(getAllImportsFor(source)); - return analysisServer.complete(source, offset); } Future fixes(String source, int offset) async { - await _checkPackageReferences(getAllImportsFor(source)); - return analysisServer.fixes(source, offset); } @@ -67,26 +55,9 @@ class Analyzer { } Future dartdoc(String source, int offset) async { - await _checkPackageReferences(getAllImportsFor(source)); - return analysisServer.dartdoc(source, offset); } - /// Check that the set of packages referenced is valid. - Future _checkPackageReferences(List imports) async { - final unsupportedImports = project.getUnsupportedImports( - imports, - sourceFiles: {kMainDart}, - ); - - if (unsupportedImports.isNotEmpty) { - final unsupportedUris = - unsupportedImports.map((import) => import.uri.stringValue).toList(); - final plural = unsupportedUris.length == 1 ? 'import' : 'imports'; - throw BadRequest('unsupported $plural ${unsupportedUris.join(', ')}'); - } - } - Future shutdown() { return analysisServer.shutdown(); } @@ -282,10 +253,7 @@ class AnalysisServerWrapper { ); } - Future analyze( - String source, { - List? imports, - }) async { + Future analyze(String source) async { final sources = _getOverlayMapWithPaths({kMainDart: source}); await _loadSources(sources); @@ -339,12 +307,57 @@ class AnalysisServerWrapper { return a.location.charStart.compareTo(b.location.charStart); }); - // Ensure we have imports if they were not passed in. - imports ??= getAllImportsFor(source); + final imports = getAllImportsFor(source); + final importIssues = []; + + for (final import in imports) { + final start = import.firstTokenAfterCommentAndMetadata; + final end = import.endToken; + + Lines? lines; + + if (import.dartImport) { + // ignore dart: imports. + } else if (import.packageImport) { + if (!isSupportedPackage(import.packageName)) { + lines ??= Lines(source); + + importIssues.add(api.AnalysisIssue( + kind: 'warning', + message: "Unsupported package: 'package:${import.packageName}'.", + location: api.Location( + charStart: start.charOffset, + charLength: end.charEnd - start.charOffset, + line: lines.lineForOffset(start.charOffset), + column: lines.columnForOffset(start.charOffset), + ), + )); + } + } else { + lines ??= Lines(source); + + importIssues.add(api.AnalysisIssue( + kind: 'error', + message: 'Import type not supported.', + location: api.Location( + charStart: start.charOffset, + charLength: end.charEnd - start.charOffset, + line: lines.lineForOffset(start.charOffset), + column: lines.columnForOffset(start.charOffset), + ), + )); + } + } return api.AnalysisResponse( - issues: issues, - packageImports: filterSafePackages(imports), + issues: [ + ...importIssues, + ...issues, + ], + packageImports: imports + .where((import) => import.packageImport) + .map((import) => import.packageName) + .toList(), ); } diff --git a/pkgs/dart_services/lib/src/pub.dart b/pkgs/dart_services/lib/src/pub.dart index cba93782a..ab921844e 100644 --- a/pkgs/dart_services/lib/src/pub.dart +++ b/pkgs/dart_services/lib/src/pub.dart @@ -67,13 +67,10 @@ Map packageVersionsFromPubspecLock(String templatePath) { return packageVersions; } -/// Returns the names of packages that are referenced in this collection. -/// These package names are sanitized defensively. -List filterSafePackages(List imports) { - return imports - .where((import) => !import.uri.stringValue!.startsWith('package:../')) - .map((import) => Uri.parse(import.uri.stringValue!)) - .where((uri) => uri.scheme == 'package' && uri.pathSegments.isNotEmpty) - .map((uri) => uri.pathSegments.first) - .toList(); +extension ImportDirectiveExtension on ImportDirective { + bool get dartImport => Uri.parse(uri.stringValue!).scheme == 'dart'; + + bool get packageImport => Uri.parse(uri.stringValue!).scheme == 'package'; + + String get packageName => Uri.parse(uri.stringValue!).pathSegments.first; } diff --git a/pkgs/dart_services/lib/src/utils.dart b/pkgs/dart_services/lib/src/utils.dart index 5f147bc6d..835bb0738 100644 --- a/pkgs/dart_services/lib/src/utils.dart +++ b/pkgs/dart_services/lib/src/utils.dart @@ -134,3 +134,37 @@ class ClosureTask extends SchedulerTask { /// This pattern is essentially "possibly some letters and colons, followed by a /// slash, followed by non-whitespace." final _possiblePathPattern = RegExp(r'[a-zA-Z:]*\/\S*'); + +class Lines { + final _starts = []; + + Lines(String source) { + final units = source.codeUnits; + var nextIsEol = true; + for (var i = 0; i < units.length; i++) { + if (nextIsEol) { + nextIsEol = false; + _starts.add(i); + } + if (units[i] == 10) nextIsEol = true; + } + } + + /// Return the 1-based line number. + int lineForOffset(int offset) { + if (_starts.isEmpty) return 1; + for (var i = 1; i < _starts.length; i++) { + if (offset < _starts[i]) return i; + } + return _starts.length; + } + + /// Return the 1-based column number. + int columnForOffset(int offset) { + if (_starts.isEmpty) return 1; + for (var i = _starts.length - 1; i >= 0; i--) { + if (offset >= _starts[i]) return offset - _starts[i] + 1; + } + return 1; + } +} diff --git a/pkgs/dart_services/test/pub_test.dart b/pkgs/dart_services/test/pub_test.dart index 9855ca741..0b7320fe2 100644 --- a/pkgs/dart_services/test/pub_test.dart +++ b/pkgs/dart_services/test/pub_test.dart @@ -70,58 +70,5 @@ void main() { } ])); }); }); - - group('filterSafePackagesFromImports', () { - test('empty', () { - const source = '''import 'package:'; -void main() { } -'''; - expect(filterSafePackages(getAllImportsFor(source)), isEmpty); - }); - - test('simple', () { - const source = ''' -import 'package:foo/foo.dart'; -import 'package:bar/bar.dart'; -void main() { } -'''; - expect(filterSafePackages(getAllImportsFor(source)), - unorderedEquals(['foo', 'bar'])); - }); - - test('defensive', () { - const source = ''' -library woot; -import 'dart:math'; -import 'package:../foo/foo.dart'; -void main() { } -'''; - final imports = getAllImportsFor(source); - expect(imports, hasLength(2)); - expect(imports[0].uri.stringValue, equals('dart:math')); - expect(imports[1].uri.stringValue, equals('package:../foo/foo.dart')); - expect(filterSafePackages(imports), isEmpty); - }); - - test('negative dart import', () { - const source = ''' -import 'dart:../bar.dart'; -'''; - final imports = getAllImportsFor(source); - expect(imports, hasLength(1)); - expect(imports.single.uri.stringValue, equals('dart:../bar.dart')); - expect(filterSafePackages(imports), isEmpty); - }); - - test('negative path import', () { - const source = ''' -import '../foo.dart'; -'''; - final imports = getAllImportsFor(source); - expect(imports, hasLength(1)); - expect(imports.single.uri.stringValue, equals('../foo.dart')); - expect(filterSafePackages(imports), isEmpty); - }); - }); }); } diff --git a/pkgs/dart_services/test/server_v3_test.dart b/pkgs/dart_services/test/server_test.dart similarity index 94% rename from pkgs/dart_services/test/server_v3_test.dart rename to pkgs/dart_services/test/server_test.dart index fd6e8923d..d0fa43160 100644 --- a/pkgs/dart_services/test/server_v3_test.dart +++ b/pkgs/dart_services/test/server_test.dart @@ -12,7 +12,7 @@ import 'package:test/test.dart'; void main() => defineTests(); void defineTests() { - group('server v3', () { + group('server', () { late final Sdk sdk; late final EndpointsServer server; late final Client httpClient; @@ -98,6 +98,22 @@ void main() { expect(issue.location.line, 2); }); + test('analyze unsupported import', () async { + final result = await client.analyze(SourceRequest(source: r''' +import 'package:foo_bar/foo_bar.dart'; + +void main() => print('hello world'); +''')); + + expect(result.issues, isNotEmpty); + + final issue = result.issues.first; + expect(issue.kind, 'warning'); + expect( + issue.message, contains("Unsupported package: 'package:foo_bar'.")); + expect(issue.location.line, 1); + }); + test('complete', () async { final result = await client.complete(SourceRequest(source: ''' void main() { diff --git a/pkgs/dart_services/test/utils_test.dart b/pkgs/dart_services/test/utils_test.dart index 68d4e6644..2cacfa7f2 100644 --- a/pkgs/dart_services/test/utils_test.dart +++ b/pkgs/dart_services/test/utils_test.dart @@ -62,4 +62,42 @@ void defineTests() { ); }); }); + + group('Lines', () { + test('empty string', () { + final lines = Lines(''); + expect(lines.lineForOffset(0), 1); + expect(lines.lineForOffset(1), 1); + expect(lines.columnForOffset(0), 1); + expect(lines.columnForOffset(1), 1); + }); + + test('lineForOffset', () { + final lines = Lines('one\ntwo\nthree'); + expect(lines.lineForOffset(0), 1); + expect(lines.lineForOffset(1), 1); + expect(lines.lineForOffset(2), 1); + expect(lines.lineForOffset(3), 1); + expect(lines.lineForOffset(4), 2); + expect(lines.lineForOffset(5), 2); + expect(lines.lineForOffset(6), 2); + expect(lines.lineForOffset(7), 2); + expect(lines.lineForOffset(8), 3); + expect(lines.lineForOffset(9), 3); + expect(lines.lineForOffset(10), 3); + expect(lines.lineForOffset(11), 3); + expect(lines.lineForOffset(12), 3); + expect(lines.lineForOffset(13), 3); + + expect(lines.lineForOffset(14), 3); + }); + + test('columnForOffset', () { + final lines = Lines('one\ntwo\nthree'); + expect(lines.columnForOffset(0), 1); + expect(lines.columnForOffset(1), 2); + expect(lines.columnForOffset(2), 3); + expect(lines.columnForOffset(3), 4); + }); + }); }