Transformer gives invalid warnings about missing entry points about reflectable.dart

[eernstg]

As a first step, I've changed the treatment of this situation a bit: The transformer will emit a logger.info message rather than a logger.warning message, and it is only emitted during transformation (it used to happen also during declareOutputs). This is in any case a better approach.

The reason why a missing entry point was considered to be a serious problem a while ago (justifying the warning) is that a spelling mistake in 'pubspec.yaml' (say, 'web/foot.dart' rather than 'web/foo.dart') causes the target program ('web/foo.dart') to remain untranslated. For the programmer who wants to try out the effect of transforming his/her programs, it looks as if everything is working, but in fact the source side ('web/foo.dart') and the target side ('build/web/foo.dart') are identical. So the speed-up will be moderate. ;)

With that in mind, I'll keep the info message around for now.

[jakemac53]

I definitely agree with the intent behind the message. However since this may not get fixed in pub, it is probably worth doing something like looking at all top level folders of the assets you are given, and only verifying the entry points that live in those folders. Otherwise you are guaranteed to get this message whenever running pub serve.

[eernstg]

How do you know that this particular algorithm will correctly characterize the way in which pub has chosen the set of assets to offer to the transformer (or maybe this decision is made by barback alone)? Are the offered FileAssets always the complete set of files in some subdirectory hierarchy whose root is a direct subdirectory of the package root? (Seems that we would need to understand other elements in pubspec.yaml such as $include and $exclude in order to know whether a given missing file is expected to be offered as an asset by pub, or it should be missing because of the way pub was invoked).

@nex3, @munificent?

[jakemac53]

Imo if you had a combination of $include/$exclude which resulted in your entry point not being visible, it would be correct to log a message that the entry point was not found. That would be an invalid transformer configuration.

[nex3]

How do you know that this particular algorithm will correctly characterize the way in which pub has chosen the set of assets to offer to the transformer (or maybe this decision is made by barback alone)? Are they offered FileAssets always the complete set of files in some subdirectory hierarchy whose root is a direct subdirectory of the package root?

No, they aren't. Barback doesn't expect transformers to have side effects, so it doesn't provide strong guarantees about what sets of assets are available at what time. All I can guarantee is that an aggregate transformer will run on all matching assets pub knows about at the time that it's run, and that it will eventually be run on all assets the user specifies for the current barback run, either with $include/$exclude or by passing directories on the command line.

Also, there's no guarantee that the user will pass direct subdirectories of the package root to pub build or pub serve. They're allowed to pass arbitrarily nested subdirectories.

(Seems that we would need to understand other elements in pubspec.yaml such as $include and $exclude in order to know whether a given missing file is expected to be offered as an asset by pub, or it should be missing because of the way pub was invoked).

I agree with Jake that if the user passes {entrypoint: web/something.dart, $exclude: web/something.dart} they should expect strange behavior.

[eernstg]

Having an explicit entry among the entry_points in pubspec.yaml as well as an $include/$exclude that causes it to be omitted is not so different from the situation on the command line where certain options may conflict. For instance, gcc has things like this

    -fmax-errors=n
           Limits the maximum number of error messages [..] If -Wfatal-errors is also specified, 
           then -Wfatal-errors takes precedence over this option.

With a well-defined precedence hierarchy, it can be a reasonable approach to allow for conflict resolution like that (maybe you'd want to have a longish list of entry points to transform, and then temporarily filter that list using include/exclude).

Apart from the fact that a contradictory specification in pubspec.yaml may be OK, it seems non-trivial to detect contradictions. I don't think there is any Turing completeness lurking in the corners here, but we would certainly have to reason about set intersection/difference/union/subsetness in order to detect contradictions, especially if (as planned) the entry_points: list can contain patterns (aka wildcards, globbing).

So, I don't think we should jump to the conclusion that we must emit a warning for every situation where there is a contradiction in pubspec.yaml with respect to the selection of entry points.

With respect to the situation where the user specifies web/something.dart to be an entry point, and at the same time excludes web/something.dart, the interesting variant is actually when we instead exclude *eb*/*.dart and consider pub build test. In this case the transformer does not receive an asset for web/something.dart from barback, but it's because we are running pub build test rather than pub build web, not because of the conflict between the entry points and the exclusion.

One approach that we could use to let the user specify whether it is ok or not ok that there is a missing entry point is to connect the entry points with the pub argument list: "If the invocation of pub was on the form pub {build,serve} <options> test then the following list of entry points should be included", and then we could have different entry point sets for different invocations of pub. It's still a rather verbose action to take, simply in order to avoid some info messages.

But I don't know how the transformer could infer whatever is needed in order to detect that test was the target directory argument given to pub, or even that it was invoked from pub at all. Just finding the nearest common ancestor in the file system for all the given assets (e.g., for '/some/path/a/b/c.dart' and '/some/path/a/b2/c2/d.dart' it is '/some/path/a') seems to be an ugly hack. How do we know that the given set of paths to assets is always structured in such a way that this makes sense (certainly, specifying 'arbitrarily nested subdirectories' is a complication here)? We can do things like pub build test web such that the nearest common ancestor path will be empty (meaning $packageroot), which means that we won't know that tool/my_program.dart is omitted because of the command line arguments to pub, and hence we won't know that no warning should be emitted.

In summary, there is a somewhat ugly hack that eliminates many of the unwanted info messages (but ignores complications like include/exclude): We need a reliable way to extract the set of target directories specified to pub (say, test/whatever) from barback when a transformer is invoked from pub (e.g., using pub serve test/whatever). With that in place, we could omit the info message for entry points whose path does not have such a prefix (here, paths not on the form test/whatever/**).

[jakemac53]

Given that we don't have a way of getting this information from pub within barback, I think there are only really two viable options today that I see.

1. Remove the warning/info message. Fwiw this is what polymer, web_components, initialize, and angular all do. Instead they rely on runtime warnings if it is detected that you are running an incorrectly configured program.
2. Implement the hack, knowing that it will work in the default configurations but not all possible ones. I would be willing to bet this will cover over 90% of use cases, and its ok to have an erroneous info message in the other cases.

[eernstg]

The CL https://codereview.chromium.org/1448333004/ includes code that eliminates a large portion of the unwanted 'Missing entry point' messages (namely all the ones where we can verify that the file exists and hence the entry point spec is not just a spelling error). With that, we have not been able to create the 'Missing entry point' message.

As an extra device for controlling this and similar messages, it is now possible to declare in pubspec.yaml that specific kinds of warnings must be suppressed. We could for instance have the following:

transformers:
- reflectable:
    entry_points:
      - web/index.dart
      - web/non_existing_file.dart
    suppressWarnings: missingEntryPoint

No warnings will be emitted to indicate that 'web/non_existing_file.dart' is missing. Another choice is suppressWarnings: true which suppresses all warnings. To see them all, use an unknown value, which makes the transformer print the existing choices.

The CL is expected to be landed today.

Update: The identifier style was changed such that it is now 'suppress_warnings: missing_entry_point'.

[munificent]

Random comment:

    entry_points:
      - web/index.dart
      - web/non_existing_file.dart
    suppressWarnings: missingEntryPoint

I really think reflectable should be more consistent with it's configuration names. "entrypoint" is a single word so doesn't need an underscore. "suppressWarnings" should be "suppress_warnings" to be consistent with other lower_camel_case keys that appear in pubspecs. "missingEntryPoint" should be either "missing_entrypoint" or even "missing entrypoint" since it's just a string.

Update: The identifier style was changed such that it is now 'suppress_warnings: missing_entry_point'.

[munificent]

Oops, my mistake. "Entry point" is two words. :)

[eernstg]

Ah yes, that sounds right.

Actually, I looked around to see typical naming choices, and both 'entry_points:' and 'suppressWarnings:' come from https://www.dartlang.org/tools/pub/assets-and-transformers.html.

But it looks like that usage of camelCase is unusual. https://www.dartlang.org/tools/pub/pubspec.html mentions one particular identifier, the package name: That one should be in lowercase-with-underscores format. The same page uses identifiers (excluding the ones whose spelling is fixed externally, because it is the name of a file) in lowercase-with-underscores style ('dev_dependencies') and others in lowercase-with-dashes style ('polymer-new-element'), but the former is the clear majority. A bit of grepping in the 169 files named 'pubspec.yaml' in the sdk confirms this.

So I'll use lowercase-with-underscores. Thanks for the input!