Skip to content

Miscellaneous warnings enhancements #1421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Miscellaneous warnings enhancements #1421

wants to merge 1 commit into from

Conversation

jcollins-g
Copy link
Contributor

While working on #1418 I needed better warning information to trace down what changed when we upgraded analyzer. This is the result. (Examples are from angular2. @chalin)

Unresolved doc references now properly indicate when the documentation is actually inherited from something else (#1405).

 warning: unresolved doc reference [AssetId]
    from angular2.transform.reflection_remover.dart.ReflectionRemover.declareOutputs: (file:///usr/local/google/home/jcollins/dart/angular2/lib/transform/reflection_remover.dart:60:3)
    in documentation inherited from barback.transformer.declaring_transformer.DeclaringTransformer.declareOutputs: (file:///usr/local/google/home/jcollins/.pub-cache/hosted/pub.dartlang.org/barback-0.15.2+10/lib/src/transformer/declaring_transformer.dart:27:3)

For some cases, the no canonical library found warning will now indicate which element referenced it. (More cases need to be added here, but this is still an improvement.)

 warning: no canonical library found for dart-html.Element.innerHtml, not linking
    from dart-html.Element.innerHtml: (file:///usr/local/google/home/jcollins/dart/all_sdks/1.24.0-dev.4.0/lib/html/dartium/html_dartium.dart)
    referred to by angular2.security.SafeInnerHtmlDirective: (file:///usr/local/google/home/jcollins/dart/angular2/lib/src/security/safe_inner_html.dart:34:7)

@jcollins-g jcollins-g requested review from devoncarew and keertip May 12, 2017 21:24
@googlebot googlebot added the cla: yes Google CLA check succeeded. label May 12, 2017
devoncarew
devoncarew reviewed May 12, 2017
View reviewed changes
lib/src/model.dart
@@ -2047,7 +2070,7 @@ abstract class ModelElement implements Comparable, Nameable, Documentable {
bool _isLineNumberComputed = false;
@override
Tuple2<int, int> get lineAndColumn {
// TODO(jcollins-g): we should always be able to get line numbers. Why can't we, sometimes?
/// TODO(jcollins-g): implement lineAndColumn for explicit fields
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want just a line comment here (//).

devoncarew
devoncarew reviewed May 12, 2017
View reviewed changes
lib/src/model.dart
@@ -2770,13 +2787,14 @@ Map<PackageWarning, List<String>> packageWarningText = {

// Something that package warnings can be called on.
abstract class Warnable implements Locatable {
void warn(PackageWarning warning, [String message]);
void warn(PackageWarning warning, {String message, Locatable referredFrom});
}

// Something that can be located for warning purposes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is existing code, but this comment should probably be a doc comment (///).

@devoncarew
Copy link
Member

lgtm!

In a future PR, we may want to translate the path urls (file:///usr/local/google/home/jcollins/dart/angular2/lib/src/security/safe_inner_html.dart) do something slightly prettier (lib/foo/bar.dart, package:foo/bar.dart, dart:core, ...). Perhaps the analysis context could help us to get back the user facing path?

@jcollins-g
Copy link
Contributor Author

Looks like some sort of VM bug. Collecting data, but sadly the bug seems to appear rather asynchronously with anything particular happening in the code.

@jcollins-g
Copy link
Contributor Author

vm bug filed at dart-lang/sdk#29620, holding off on this PR until that's settled.

@devoncarew
Copy link
Member

I had the bot re-run the build - looks like the crash is consistent (which is surprising to me - I'd assumed it was an intermittent failure).

@jcollins-g
Copy link
Contributor Author

Yep. Crash reliably happens. I have a workaround, detailed in this comment, but in order to keep the reproduction case around, leaving this branch as is until they have a chance to fix the problem.

@jcollins-g
Copy link
Contributor Author

Going to close this PR and preserve the branch until the vm folks have a chance to fix their bug. The workaround is actually quite reasonable and is probably what dartdoc should be doing anyway, so will reopen this change in a new branch.

@jcollins-g jcollins-g closed this May 17, 2017
@jcollins-g jcollins-g mentioned this pull request May 17, 2017
Merged
@kevmoo kevmoo deleted the warnings-update branch May 19, 2017 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devoncarew devoncarew devoncarew left review comments

@keertip keertip Awaiting requested review from keertip

Assignees
No one assigned
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcollins-g @devoncarew @googlebot