From 9a78604d985f9131b7932cd89199a5629f1d9fe2 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Thu, 16 Apr 2020 19:07:30 +0200 Subject: [PATCH] Fix issues with new inference, enable it by default --- analysis_options.yaml | 1 + .../docs/Advanced Features/builder_options.md | 11 ++--- moor/build.yaml | 1 - moor_generator/lib/src/analyzer/options.dart | 11 +++-- .../lib/src/analyzer/options.g.dart | 8 ++-- moor_generator/lib/src/analyzer/session.dart | 2 +- .../moor/moor_ffi_extension_test.dart | 5 +-- .../inference_test.dart} | 4 +- sqlparser/CHANGELOG.md | 2 + sqlparser/lib/src/analysis/context.dart | 8 +--- .../src/analysis/types2/graph/type_graph.dart | 27 +++++++----- .../analysis/types2/resolving_visitor.dart | 43 ++++++++++++++++--- sqlparser/lib/src/engine/options.dart | 8 ++-- sqlparser/lib/src/engine/sql_engine.dart | 6 +-- .../test/analysis/types/resolver_test.dart | 2 +- .../test/analysis/types2/misc_cases_test.dart | 4 +- sqlparser/test/engine/module/json1_test.dart | 6 +-- 17 files changed, 88 insertions(+), 61 deletions(-) rename moor_generator/test/analyzer/{experimental_inference_test.dart => sql_queries/inference_test.dart} (94%) diff --git a/analysis_options.yaml b/analysis_options.yaml index cd860678..a2996adc 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -7,6 +7,7 @@ analyzer: unused_local_variable: error dead_code: error public_member_api_docs: ignore # turned on by user-facing subpackages + deprecated_member_use_from_same_package: ignore exclude: - "**/*.g.dart" # Will be analyzed anyway, nobody knows why ¯\_(ツ)_/¯. We're only analyzing lib/ and test/ as a workaround diff --git a/docs/content/en/docs/Advanced Features/builder_options.md b/docs/content/en/docs/Advanced Features/builder_options.md index cfacaaf0..0e766baa 100644 --- a/docs/content/en/docs/Advanced Features/builder_options.md +++ b/docs/content/en/docs/Advanced Features/builder_options.md @@ -52,19 +52,14 @@ At the moment, moor supports these options: column constraint (e.g. `user_name VARCHAR NOT NULL JSON KEY userName`) * `generate_connect_constructor`: Generate necessary code to support the [isolate runtime]({{< relref "isolates.md" >}}). This is a build option because isolates are still experimental. This will be the default option eventually. -* `use_experimental_inference`: Use a new, experimental type inference algorithm when analyzing sql statements. The - algorithm is designed to yield more accurate results on nullability and complex constructs. Note that it's in a - preview state at the moment, which means that generated code might change after a minor update. * `sqlite_modules`: This list can be used to enable sqlite extensions, like those for json or full-text search. Modules have to be enabled explicitly because they're not supported on all platforms. See the following section for details. -* `use_experimental_inference`: Enables a new type inference algorithm for sql statements. - The new algorithm is much better at handling complex statements and nullability. - However, it's still in development and may not work in all cases yet. Please report any issues you can find. - __Warning:__ The new type inference algorithm is in development and does not obey to semantic versioning. - Results and generated code might change in moor versions not declared as breaking. * `eagerly_load_dart_ast`: Moor's builder will load the resolved AST whenever it encounters a Dart file, instead of lazily when it reads a table. This is used to investigate rare builder crashes. +* `legacy_type_inference`: Use the old type inference from moor 1 and 2. Note that `use_experimental_inference` + is now the default and no longer exists. + If you're using this flag, please open an issue and explain how the new inference isn't working for you, thanks! ## Available extensions diff --git a/moor/build.yaml b/moor/build.yaml index 63e6357b..6829ee04 100644 --- a/moor/build.yaml +++ b/moor/build.yaml @@ -8,7 +8,6 @@ targets: generate_connect_constructor: true compact_query_methods: true write_from_json_string_constructor: true - use_experimental_inference: true eagerly_load_dart_ast: true sqlite_modules: - json1 diff --git a/moor_generator/lib/src/analyzer/options.dart b/moor_generator/lib/src/analyzer/options.dart index f6907568..0d360fd6 100644 --- a/moor_generator/lib/src/analyzer/options.dart +++ b/moor_generator/lib/src/analyzer/options.dart @@ -54,11 +54,10 @@ class MoorOptions { @JsonKey(name: 'generate_connect_constructor', defaultValue: false) final bool generateConnectConstructor; - /// Whether the new, experimental type inference engine should be used. - /// - /// The new engine is experimental at the moment. - @JsonKey(name: 'use_experimental_inference', defaultValue: false) - final bool useExperimentalInference; + /// Whether the old, legacy type inference engine should be used when + /// analyzing custom sql queries. + @JsonKey(name: 'legacy_type_inference', defaultValue: false) + final bool legacyTypeInference; @JsonKey(name: 'sqlite_modules', defaultValue: []) final List modules; @@ -77,7 +76,7 @@ class MoorOptions { this.useDataClassNameForCompanions = false, this.useColumnNameAsJsonKeyWhenDefinedInMoorFile = false, this.generateConnectConstructor = false, - this.useExperimentalInference = false, + this.legacyTypeInference = false, this.eagerlyLoadDartAst = false, this.modules = const []}); diff --git a/moor_generator/lib/src/analyzer/options.g.dart b/moor_generator/lib/src/analyzer/options.g.dart index e1101d8c..99b3c034 100644 --- a/moor_generator/lib/src/analyzer/options.g.dart +++ b/moor_generator/lib/src/analyzer/options.g.dart @@ -16,7 +16,7 @@ MoorOptions _$MoorOptionsFromJson(Map json) { 'use_data_class_name_for_companions', 'use_column_name_as_json_key_when_defined_in_moor_file', 'generate_connect_constructor', - 'use_experimental_inference', + 'legacy_type_inference', 'sqlite_modules', 'eagerly_load_dart_ast' ]); @@ -44,9 +44,9 @@ MoorOptions _$MoorOptionsFromJson(Map json) { generateConnectConstructor: $checkedConvert( json, 'generate_connect_constructor', (v) => v as bool) ?? false, - useExperimentalInference: $checkedConvert( - json, 'use_experimental_inference', (v) => v as bool) ?? - false, + legacyTypeInference: + $checkedConvert(json, 'legacy_type_inference', (v) => v as bool) ?? + false, eagerlyLoadDartAst: $checkedConvert(json, 'eagerly_load_dart_ast', (v) => v as bool) ?? false, diff --git a/moor_generator/lib/src/analyzer/session.dart b/moor_generator/lib/src/analyzer/session.dart index 48781357..56cf88c3 100644 --- a/moor_generator/lib/src/analyzer/session.dart +++ b/moor_generator/lib/src/analyzer/session.dart @@ -44,7 +44,7 @@ class MoorSession { if (options.hasModule(SqlModule.json1)) const Json1Extension(), if (options.hasModule(SqlModule.moor_ffi)) const MoorFfiExtension(), ], - enableExperimentalTypeInference: options.useExperimentalInference, + useLegacyTypeInference: options.legacyTypeInference, ); return SqlEngine(sqlOptions); diff --git a/moor_generator/test/analyzer/moor/moor_ffi_extension_test.dart b/moor_generator/test/analyzer/moor/moor_ffi_extension_test.dart index 8d1da77e..aa912857 100644 --- a/moor_generator/test/analyzer/moor/moor_ffi_extension_test.dart +++ b/moor_generator/test/analyzer/moor/moor_ffi_extension_test.dart @@ -89,10 +89,7 @@ import 'a.moor'; wrongArgs: SELECT sin(oid, foo) FROM numbers; ''' }, - options: const MoorOptions( - useExperimentalInference: true, - modules: [SqlModule.moor_ffi], - ), + options: const MoorOptions(modules: [SqlModule.moor_ffi]), ); final fileA = await state.analyze('package:foo/a.moor'); diff --git a/moor_generator/test/analyzer/experimental_inference_test.dart b/moor_generator/test/analyzer/sql_queries/inference_test.dart similarity index 94% rename from moor_generator/test/analyzer/experimental_inference_test.dart rename to moor_generator/test/analyzer/sql_queries/inference_test.dart index 84f83bf2..47ad4767 100644 --- a/moor_generator/test/analyzer/experimental_inference_test.dart +++ b/moor_generator/test/analyzer/sql_queries/inference_test.dart @@ -3,7 +3,7 @@ import 'package:moor_generator/src/analyzer/options.dart'; import 'package:moor_generator/src/analyzer/runner/results.dart'; import 'package:test/test.dart'; -import 'utils.dart'; +import '../utils.dart'; void main() { test('experimental inference - integration test', () async { @@ -35,7 +35,7 @@ totalDurationByArtist: INNER JOIN tracks ON tracks.album = albums.id GROUP BY artists.id; ''' - }, options: const MoorOptions(useExperimentalInference: true)); + }, options: const MoorOptions()); final file = await state.analyze('package:foo/a.moor'); final result = file.currentResult as ParsedMoorFile; diff --git a/sqlparser/CHANGELOG.md b/sqlparser/CHANGELOG.md index cfbb2011..7f00e102 100644 --- a/sqlparser/CHANGELOG.md +++ b/sqlparser/CHANGELOG.md @@ -5,6 +5,8 @@ tables with a comma will now be parsed as a `JoinClause`. - Changed `SelectStatementAsSource.statement` from `SelectStatement` to `BaseSelectStatement` and allow compound select statements to appear in a `FROM` clause +- The new type inference engine is now enabled by default and the `enableExperimentalTypeInference` option + has been removed. To continue using the old engine, the `useLegacyTypeInference` flag can be used. ## 0.7.0 diff --git a/sqlparser/lib/src/analysis/context.dart b/sqlparser/lib/src/analysis/context.dart index 17b8b940..1ad3d0ba 100644 --- a/sqlparser/lib/src/analysis/context.dart +++ b/sqlparser/lib/src/analysis/context.dart @@ -29,6 +29,7 @@ class AnalysisContext { /// A resolver that can be used to obtain the type of a [Typeable]. This /// mostly applies to [Expression]s, [Reference]s, [Variable]s and /// [ResultSet.resolvedColumns] of a select statement. + @Deprecated('Used for legacy types only - consider migrating to types2') /* late final */ TypeResolver types; /// Experimental new type resolver with better support for nullability and @@ -38,18 +39,13 @@ class AnalysisContext { /// a [Variable] and [ResultSet.resolvedColumns] may be resolved or inferred. /// /// This field is null when experimental type inference is disabled. - /// - /// Please note that types2 is experimental at the moment. Changes to how - /// [types] resolves types are considered breaking and are handled - /// accordingly. [types2] may change results in any update. - @experimental TypeInferenceResults types2; /// Constructs a new analysis context from the AST and the source sql. AnalysisContext(this.root, this.sql, this.engineOptions, {AnalyzeStatementOptions stmtOptions, this.schemaSupport}) : stmtOptions = stmtOptions ?? const AnalyzeStatementOptions() { - if (!engineOptions.enableExperimentalTypeInference) { + if (engineOptions.useLegacyTypeInference) { types = TypeResolver(this, engineOptions); } } diff --git a/sqlparser/lib/src/analysis/types2/graph/type_graph.dart b/sqlparser/lib/src/analysis/types2/graph/type_graph.dart index 29f4c24a..97d95d46 100644 --- a/sqlparser/lib/src/analysis/types2/graph/type_graph.dart +++ b/sqlparser/lib/src/analysis/types2/graph/type_graph.dart @@ -9,7 +9,7 @@ class TypeGraph { final List _relations = []; final Map> _edges = {}; - final Set _multiSources = {}; + final Set _candidateForLaxMultiPropagation = {}; final List _defaultTypes = []; ResolvedType operator [](Typeable t) { @@ -51,14 +51,17 @@ class TypeGraph { void performResolve() { _indexRelations(); - final queue = List.of(_knownTypes.keys); + var queue = List.of(_knownTypes.keys); while (queue.isNotEmpty) { _propagateTypeInfo(queue, queue.removeLast()); } - // propagate many-to-one sources where we don't know each source type - for (final remaining in _multiSources) { - _propagateManyToOne(remaining, queue); + // propagate many-to-one sources where we don't know each source type, but + // some of them. + queue = List.of(_candidateForLaxMultiPropagation); + while (queue.isNotEmpty) { + _propagateTypeInfo(queue, queue.removeLast(), + laxMultiSourcePropagation: true); } // apply default types @@ -77,7 +80,8 @@ class TypeGraph { } } - void _propagateTypeInfo(List resolved, Typeable t) { + void _propagateTypeInfo(List resolved, Typeable t, + {bool laxMultiSourcePropagation = false}) { if (!_edges.containsKey(t)) return; // propagate changes @@ -93,10 +97,14 @@ class TypeGraph { } else if (edge is CopyAndCast) { _copyType(resolved, t, edge.target, this[t].cast(edge.cast)); } else if (edge is MultiSourceRelation) { - // handle many-to-one changes, if all targets have been resolved - if (edge.from.every(knowsType)) { - _multiSources.remove(edge); + // handle many-to-one changes, if all targets have been resolved or + // lax handling is enabled. + if (laxMultiSourcePropagation || edge.from.every(knowsType)) { _propagateManyToOne(edge, resolved); + + _candidateForLaxMultiPropagation.removeAll(edge.from); + } else { + _candidateForLaxMultiPropagation.add(t); } } } @@ -165,7 +173,6 @@ class TypeGraph { } void putAll(MultiSourceRelation r) { - _multiSources.add(r); for (final element in r.from) { put(element, r); } diff --git a/sqlparser/lib/src/analysis/types2/resolving_visitor.dart b/sqlparser/lib/src/analysis/types2/resolving_visitor.dart index 9cad3902..9d7c5e75 100644 --- a/sqlparser/lib/src/analysis/types2/resolving_visitor.dart +++ b/sqlparser/lib/src/analysis/types2/resolving_visitor.dart @@ -11,6 +11,8 @@ const _expectString = ExactTypeExpectation.laxly(_textType); class TypeResolver extends RecursiveVisitor { final TypeInferenceSession session; + final Set _handledColumns = {}; + TypeResolver(this.session); void run(AstNode root) { @@ -414,6 +416,10 @@ class TypeResolver extends RecursiveVisitor { } } + FunctionHandler /*?*/ _functionHandlerFor(ExpressionInvocation e) { + return session.options.addedFunctions[e.name.toLowerCase()]; + } + ResolvedType _resolveInvocation(ExpressionInvocation e) { final params = e.expandParameters(); void nullableIfChildIs() { @@ -516,11 +522,9 @@ class TypeResolver extends RecursiveVisitor { return null; } - final addedFunctions = session.options.addedFunctions; - if (addedFunctions.containsKey(lowercaseName)) { - return addedFunctions[lowercaseName] - .inferReturnType(session.context, e, params) - ?.type; + final extensionHandler = _functionHandlerFor(e); + if (extensionHandler != null) { + return extensionHandler.inferReturnType(session.context, e, params)?.type; } session.context.reportError(AnalysisError( @@ -542,6 +546,25 @@ class TypeResolver extends RecursiveVisitor { visited.add(secondParam); } + final extensionHandler = + e is ExpressionInvocation ? _functionHandlerFor(e) : null; + if (extensionHandler != null) { + for (final arg in params) { + if (arg is! Expression) continue; + + final expressionArgument = arg as Expression; + + final result = extensionHandler.inferArgumentType( + session.context, e, expressionArgument); + final type = result?.type; + if (type != null) { + session._markTypeResolved(expressionArgument, type); + } + + visited.add(expressionArgument); + } + } + return visited; } @@ -558,12 +581,18 @@ class TypeResolver extends RecursiveVisitor { } void _handleColumn(Column column) { - if (session.graph.knowsType(column)) return; + if (session.graph.knowsType(column) || _handledColumns.contains(column)) { + return; + } + _handledColumns.add(column); if (column is TableColumn) { session._markTypeResolved(column, column.type); } else if (column is ExpressionColumn) { _lazyCopy(column, column.expression); + } else if (column is CompoundSelectColumn) { + session._addRelation(CopyEncapsulating(column, column.columns)); + column.columns.forEach(_handleColumn); } else if (column is DelegatedColumn && column.innerColumn != null) { _handleColumn(column.innerColumn); _lazyCopy(column, column.innerColumn); @@ -605,5 +634,7 @@ class _ResultColumnVisitor extends RecursiveVisitor { if (stmt.resolvedColumns != null) { stmt.resolvedColumns.forEach(resolver._handleColumn); } + + visitChildren(stmt, arg); } } diff --git a/sqlparser/lib/src/engine/options.dart b/sqlparser/lib/src/engine/options.dart index 21755151..f60eea5b 100644 --- a/sqlparser/lib/src/engine/options.dart +++ b/sqlparser/lib/src/engine/options.dart @@ -6,8 +6,10 @@ class EngineOptions { /// extensions enabled. final bool useMoorExtensions; - /// Enables the new, experimental type inference. - final bool enableExperimentalTypeInference; + /// Whether the old type inference algorithm should be used. + /// + /// Defaults to false. + final bool useLegacyTypeInference; /// All [Extension]s that have been enabled in this sql engine. final List enabledExtensions; @@ -24,7 +26,7 @@ class EngineOptions { EngineOptions({ this.useMoorExtensions = false, this.enabledExtensions = const [], - this.enableExperimentalTypeInference = false, + this.useLegacyTypeInference = false, }); void addFunctionHandler(FunctionHandler handler) { diff --git a/sqlparser/lib/src/engine/sql_engine.dart b/sqlparser/lib/src/engine/sql_engine.dart index ad67e698..8525d30e 100644 --- a/sqlparser/lib/src/engine/sql_engine.dart +++ b/sqlparser/lib/src/engine/sql_engine.dart @@ -190,13 +190,13 @@ class SqlEngine { ..acceptWithoutArg(ColumnResolver(context)) ..acceptWithoutArg(ReferenceResolver(context)); - if (options.enableExperimentalTypeInference) { + if (options.useLegacyTypeInference) { + node.acceptWithoutArg(TypeResolvingVisitor(context)); + } else { final session = t2.TypeInferenceSession(context, options); final resolver = t2.TypeResolver(session); resolver.run(node); context.types2 = session.results; - } else { - node.acceptWithoutArg(TypeResolvingVisitor(context)); } node.acceptWithoutArg(LintingVisitor(options, context)); diff --git a/sqlparser/test/analysis/types/resolver_test.dart b/sqlparser/test/analysis/types/resolver_test.dart index d5989541..d23b36ac 100644 --- a/sqlparser/test/analysis/types/resolver_test.dart +++ b/sqlparser/test/analysis/types/resolver_test.dart @@ -42,7 +42,7 @@ Map _types = { void main() { _types.forEach((sql, resolvedType) { test('types: resolves in $sql', () { - final engine = SqlEngine() + final engine = SqlEngine(EngineOptions(useLegacyTypeInference: true)) ..registerTable(demoTable) ..registerTable(anotherTable); final content = engine.analyze(sql); diff --git a/sqlparser/test/analysis/types2/misc_cases_test.dart b/sqlparser/test/analysis/types2/misc_cases_test.dart index d7044d50..b4bdf365 100644 --- a/sqlparser/test/analysis/types2/misc_cases_test.dart +++ b/sqlparser/test/analysis/types2/misc_cases_test.dart @@ -39,9 +39,7 @@ const Map _types = { }; SqlEngine _spawnEngine() { - return SqlEngine(EngineOptions(enableExperimentalTypeInference: true)) - ..registerTable(demoTable) - ..registerTable(anotherTable); + return SqlEngine()..registerTable(demoTable)..registerTable(anotherTable); } void main() { diff --git a/sqlparser/test/engine/module/json1_test.dart b/sqlparser/test/engine/module/json1_test.dart index ba3c6fac..b5ea1976 100644 --- a/sqlparser/test/engine/module/json1_test.dart +++ b/sqlparser/test/engine/module/json1_test.dart @@ -2,18 +2,18 @@ import 'package:sqlparser/sqlparser.dart'; import 'package:test/test.dart'; void main() { - group('json with stable inference', () { + group('json with legacy inference', () { _runTests(false); }); - group('json with types2', () { + group('json with new type inference', () { _runTests(true); }); } void _runTests(bool types2) { final engine = SqlEngine(EngineOptions( - enableExperimentalTypeInference: types2, + useLegacyTypeInference: !types2, enabledExtensions: const [Json1Extension()], )); // add user (name, phone) table