Use FastaMessage instead of String. Part 1.

Use FastaMessage instead of String. Part 1.

See #30080.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/b023d925eab4ec717f70071afaa34f51ef9dc4b5

[ahe, 2017-07-07 15:15:33]

Patchset #1 (id:1) has been deleted

[ahe, 2017-07-07 15:25:41]

ahe@google.com changed reviewers:
+ sigmund@google.com

[ahe, 2017-07-07 15:25:42]

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta_codes.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:44: class BoundFastaCode extends
FastaCode<BoundFastaCode> {
I'm thinking about renaming this to NoArgumentsFastaCode

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:76: typedef BoundFastaMessage =
FastaMessage Function(Uri, int);
I'm thinking about renaming this to BoundCode.

[Siggi Cherem (dart-lang), 2017-07-07 21:17:08]

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:81: void
addCompileTimeError(BoundFastaMessage bind, int charOffset, Uri uri,
I feel we are are overloading the term `bind`:

  * bind in UnboundFastaCode converts unbound-code to produce a bound-message
  * bind here converts a bound-message to a message with location

A small fix would be to use a different variable name here, like "message", but
I think it is a confusing name to use as a function target.

I believe it makes sense to do some broader renames of the types. One option is
to rename "BoundFastaMessage" to something that is more like a verb like
"FastaMessageCreator" and use a variable name that implies the action (e.g.
"generateMessage"). But I feel things will get cleaner if we represent
`BoundFastaMessage` as an object and not a function.  I made a more detailed
suggestion in fasta_codes.dart, take a look there.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta_codes.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:11: abstract class FastaCode<T> {
seems you can drop the T here now.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:44: class BoundFastaCode extends
FastaCode<BoundFastaCode> {
On 2017/07/07 15:25:42, ahe wrote:
> I'm thinking about renaming this to NoArgumentsFastaCode

+1 on doing some renames.

Some alternative suggestions:

  FastaCode => MessageBase or MessageDescriptor
  typedef BoundFastaCode => class Message (use an object)
  UnboundFastaCode => MessageTemplate
  FastaMessage => MessageWithLocation or BoundMessage or LocatedMessage
  bind => eval

So the code would look like this:

class MessageBase {
  final String name;
  final String template;
  ...
}

class MessageTemplate<T> extends MessageBase {
   /// A function that takes all the arguments needed by the MessageTemplate
   /// to produce an actual Message.
   T eval;
}

class Message extends MessageBase {
   ...
   LocatedMessage at(Uri file, int offset) => new LocatedMessage(uri, offset,
this);
}

class LocatedMessage {
  Uri uri;
  int offset;
  Message message;
}


My rationale:
  - no need to use the Fasta prefix IMO
  - MessageBase conveys that this is abstract, there are two implementations,
depending on whether arguments are expected.
  - "Template" implies arguments that are not bound
  - putting a message in a location is just wrapping an existing message.


Thoughts?

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:66: final String tip;
could we turn this into:

String get tip => code.tip;

?

[ahe, 2017-07-10 13:39:35]

Patchset #2 (id:40001) has been deleted

[ahe, 2017-07-10 13:43:50]

PTAL

I suggest you ignore the old patch set.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/builder/library_builder.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/builder/library_builder.dart:81: void
addCompileTimeError(BoundFastaMessage bind, int charOffset, Uri uri,
On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> I feel we are are overloading the term `bind`:
> 
>   * bind in UnboundFastaCode converts unbound-code to produce a bound-message
>   * bind here converts a bound-message to a message with location
> 
> A small fix would be to use a different variable name here, like "message",
but
> I think it is a confusing name to use as a function target.
> 
> I believe it makes sense to do some broader renames of the types. One option
is
> to rename "BoundFastaMessage" to something that is more like a verb like
> "FastaMessageCreator" and use a variable name that implies the action (e.g.
> "generateMessage"). But I feel things will get cleaner if we represent
> `BoundFastaMessage` as an object and not a function.  I made a more detailed
> suggestion in fasta_codes.dart, take a look there.

I agree.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta_codes.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:11: abstract class FastaCode<T> {
On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> seems you can drop the T here now.

Actually, T was essential for ensuring type safety. But it's gone now.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:44: class BoundFastaCode extends
FastaCode<BoundFastaCode> {
On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> On 2017/07/07 15:25:42, ahe wrote:
> > I'm thinking about renaming this to NoArgumentsFastaCode
> 
> +1 on doing some renames.
> 
> Some alternative suggestions:
> 
>   FastaCode => MessageBase or MessageDescriptor
>   typedef BoundFastaCode => class Message (use an object)
>   UnboundFastaCode => MessageTemplate
>   FastaMessage => MessageWithLocation or BoundMessage or LocatedMessage
>   bind => eval
> 
> So the code would look like this:
> 
> class MessageBase {
>   final String name;
>   final String template;
>   ...
> }
> 
> class MessageTemplate<T> extends MessageBase {
>    /// A function that takes all the arguments needed by the MessageTemplate
>    /// to produce an actual Message.
>    T eval;
> }
> 
> class Message extends MessageBase {
>    ...
>    LocatedMessage at(Uri file, int offset) => new LocatedMessage(uri, offset,
> this);
> }
> 
> class LocatedMessage {
>   Uri uri;
>   int offset;
>   Message message;
> }
> 
> 
> My rationale:
>   - no need to use the Fasta prefix IMO
>   - MessageBase conveys that this is abstract, there are two implementations,
> depending on whether arguments are expected.
>   - "Template" implies arguments that are not bound
>   - putting a message in a location is just wrapping an existing message.
> 
> 
> Thoughts?

I did something like that.

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:66: final String tip;
On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> could we turn this into:
> 
> String get tip => code.tip;
> 
> ?

No, the tip is also subject to template expansion.

[ahe, 2017-07-10 14:47:11]

FYI: Johnni

[Siggi Cherem (dart-lang), 2017-07-10 16:19:48]

Nice, I really like the new types and naming convention. I feel it also makes it
much cleaner where we use the messages.

lgtm!

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta_codes.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:11: abstract class FastaCode<T> {
On 2017/07/10 13:43:50, ahe wrote:
> On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> > seems you can drop the T here now.
> 
> Actually, T was essential for ensuring type safety. But it's gone now.

BTW - I was suggesting it when you didn't have anything depending on T in this
class, but to keep it in the subclasses.

Now that you have Code containing Template, and Template is parameterized,
should we add it back? For instance,

class Code<T> {
  ...
  final Template<T> template;

[ahe, 2017-07-10 16:57:26]

Thank you, Siggi!

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta_codes.dart (right):

https://codereview.chromium.org/2965393002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta_codes.dart:11: abstract class FastaCode<T> {
On 2017/07/10 16:19:47, Siggi Cherem (dart-lang) wrote:
> On 2017/07/10 13:43:50, ahe wrote:
> > On 2017/07/07 21:17:08, Siggi Cherem (dart-lang) wrote:
> > > seems you can drop the T here now.
> > 
> > Actually, T was essential for ensuring type safety. But it's gone now.
> 
> BTW - I was suggesting it when you didn't have anything depending on T in this
> class, but to keep it in the subclasses.

I know that nothing was depending on it, but for type safety, I had to have it
there.

> Now that you have Code containing Template, and Template is parameterized,
> should we add it back? For instance,
> 
> class Code<T> {
>   ...
>   final Template<T> template;

Yes. You're right.

[ahe, 2017-07-10 17:50:20]

Description was changed from

==========
Use FastaMessage instead of String. Part 1.

See #30080.
==========

to

==========
Use FastaMessage instead of String. Part 1.

See #30080.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b023d925eab4ec717f70071afaa34f51ef9dc4b5
==========

[ahe, 2017-07-10 17:50:22]

Committed patchset #4 (id:100001) manually as
b023d925eab4ec717f70071afaa34f51ef9dc4b5.