Skip to content

Typing fixes for Pontoon TypeScript migration, fix #524 #525

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

Merged
merged 6 commits into from
Apr 2, 2021
Merged

Typing fixes for Pontoon TypeScript migration, fix #524 #525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6bc646
Stricter typing for @fluent/langneg
Pike Mar 17, 2021
d4babe5
Better typing for @fluent/syntax
Pike Mar 18, 2021
77f86cb
Give input type hints to Visitor and Transformer
Pike Mar 18, 2021
b1496e3
Align AST with fluent-rs to use Union types instead of inheritance.
Pike Mar 22, 2021
1525850
Expose option type of API
Pike Mar 31, 2021
8dab470
Fixes to syntax typing
Pike Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fluent-langneg/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion fluent-langneg/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
*
*/

export {negotiateLanguages} from "./negotiate_languages";
export {
negotiateLanguages, NegotiateLanguagesOptions
} from "./negotiate_languages";
export {acceptedLanguages} from "./accepted_languages";
export {filterMatches} from "./matches";
6 changes: 3 additions & 3 deletions fluent-langneg/src/negotiate_languages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {filterMatches} from "./matches";

interface NegotiateLanguagesOptions {
export interface NegotiateLanguagesOptions {
strategy?: "filtering" | "matching" | "lookup";
defaultLocale?: string;
}
Expand Down Expand Up @@ -49,8 +49,8 @@ interface NegotiateLanguagesOptions {
* This strategy requires defaultLocale option to be set.
*/
export function negotiateLanguages(
requestedLocales: Array<string>,
availableLocales: Array<string>,
requestedLocales: Readonly<Array<string>>,
availableLocales: Readonly<Array<string>>,
{
strategy = "filtering",
defaultLocale,
Expand Down
2 changes: 1 addition & 1 deletion fluent-syntax/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

89 changes: 46 additions & 43 deletions fluent-syntax/src/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*
*/
export abstract class BaseNode {
public type = "BaseNode";
[name: string]: unknown;

equals(other: BaseNode, ignoredFields: Array<string> = ["span"]): boolean {
Expand Down Expand Up @@ -46,7 +45,7 @@ export abstract class BaseNode {
return true;
}

clone(): BaseNode {
clone(): this {
function visit(value: unknown): unknown {
if (value instanceof BaseNode) {
return value.clone();
Expand Down Expand Up @@ -79,7 +78,6 @@ function scalarsEqual(
* Base class for AST nodes which can have Spans.
*/
export abstract class SyntaxNode extends BaseNode {
public type = "SyntaxNode";
public span?: Span;

addSpan(start: number, end: number): void {
Expand All @@ -89,21 +87,20 @@ export abstract class SyntaxNode extends BaseNode {

export class Resource extends SyntaxNode {
public type = "Resource" as const;
public body: Array<Entry | Junk>;
constructor(body: Array<Entry | Junk> = []) {
public body: Array<Entry>;
constructor(body: Array<Entry> = []) {
super();
this.body = body;
}
}

/*
* An abstract base class for useful elements of Resource.body.
*/
export abstract class Entry extends SyntaxNode {
public type = "Entry";
}
export declare type Entry =
Message |
Term |
Comments |
Junk;

export class Message extends Entry {
export class Message extends SyntaxNode {
public type = "Message" as const;
public id: Identifier;
public value: Pattern | null;
Expand All @@ -124,7 +121,7 @@ export class Message extends Entry {
}
}

export class Term extends Entry {
export class Term extends SyntaxNode {
public type = "Term" as const;
public id: Identifier;
public value: Pattern;
Expand Down Expand Up @@ -155,14 +152,9 @@ export class Pattern extends SyntaxNode {
}
}

/*
* An abstract base class for elements of Patterns.
*/
export abstract class PatternElement extends SyntaxNode {
public type = "PatternElement";
}
export declare type PatternElement = TextElement | Placeable;

export class TextElement extends PatternElement {
export class TextElement extends SyntaxNode {
public type = "TextElement" as const;
public value: string;

Expand All @@ -172,7 +164,7 @@ export class TextElement extends PatternElement {
}
}

export class Placeable extends PatternElement {
export class Placeable extends SyntaxNode {
public type = "Placeable" as const;
public expression: Expression;

Expand All @@ -182,16 +174,21 @@ export class Placeable extends PatternElement {
}
}

/*
* An abstract base class for expressions.
/**
* A subset of expressions which can be used as outside of Placeables.
*/
export abstract class Expression extends SyntaxNode {
public type = "Expression";
}
export type InlineExpression =
StringLiteral |
NumberLiteral |
FunctionReference |
MessageReference |
TermReference |
VariableReference |
Placeable;
export declare type Expression = InlineExpression | SelectExpression;

// An abstract base class for Literals.
export abstract class Literal extends Expression {
public type = "Literal";
export abstract class BaseLiteral extends SyntaxNode {
public value: string;

constructor(value: string) {
Expand All @@ -204,7 +201,7 @@ export abstract class Literal extends Expression {
abstract parse(): { value: unknown }
}

export class StringLiteral extends Literal {
export class StringLiteral extends BaseLiteral {
public type = "StringLiteral" as const;

parse(): { value: string } {
Expand Down Expand Up @@ -241,7 +238,7 @@ export class StringLiteral extends Literal {
}
}

export class NumberLiteral extends Literal {
export class NumberLiteral extends BaseLiteral {
public type = "NumberLiteral" as const;

parse(): { value: number; precision: number } {
Expand All @@ -254,7 +251,9 @@ export class NumberLiteral extends Literal {
}
}

export class MessageReference extends Expression {
export declare type Literal = StringLiteral | NumberLiteral;

export class MessageReference extends SyntaxNode {
public type = "MessageReference" as const;
public id: Identifier;
public attribute: Identifier | null;
Expand All @@ -266,7 +265,7 @@ export class MessageReference extends Expression {
}
}

export class TermReference extends Expression {
export class TermReference extends SyntaxNode {
public type = "TermReference" as const;
public id: Identifier;
public attribute: Identifier | null;
Expand All @@ -284,7 +283,7 @@ export class TermReference extends Expression {
}
}

export class VariableReference extends Expression {
export class VariableReference extends SyntaxNode {
public type = "VariableReference" as const;
public id: Identifier;

Expand All @@ -294,7 +293,7 @@ export class VariableReference extends Expression {
}
}

export class FunctionReference extends Expression {
export class FunctionReference extends SyntaxNode {
public type = "FunctionReference" as const;
public id: Identifier;
public arguments: CallArguments;
Expand All @@ -306,12 +305,12 @@ export class FunctionReference extends Expression {
}
}

export class SelectExpression extends Expression {
export class SelectExpression extends SyntaxNode {
public type = "SelectExpression" as const;
public selector: Expression;
public selector: InlineExpression;
public variants: Array<Variant>;

constructor(selector: Expression, variants: Array<Variant>) {
constructor(selector: InlineExpression, variants: Array<Variant>) {
super();
this.selector = selector;
this.variants = variants;
Expand All @@ -320,11 +319,11 @@ export class SelectExpression extends Expression {

export class CallArguments extends SyntaxNode {
public type = "CallArguments" as const;
public positional: Array<Expression>;
public positional: Array<InlineExpression>;
public named: Array<NamedArgument>;

constructor(
positional: Array<Expression> = [],
positional: Array<InlineExpression> = [],
named: Array<NamedArgument> = []
) {
super();
Expand Down Expand Up @@ -381,8 +380,7 @@ export class Identifier extends SyntaxNode {
}
}

export abstract class BaseComment extends Entry {
public type = "BaseComment";
export abstract class BaseComment extends SyntaxNode {
public content: string;
constructor(content: string) {
super();
Expand All @@ -401,6 +399,11 @@ export class ResourceComment extends BaseComment {
public type = "ResourceComment" as const;
}

export declare type Comments =
Comment |
GroupComment |
ResourceComment;

export class Junk extends SyntaxNode {
public type = "Junk" as const;
public annotations: Array<Annotation>;
Expand All @@ -418,7 +421,7 @@ export class Junk extends SyntaxNode {
}

export class Span extends BaseNode {
public type = "Span";
public type = "Span" as const;
public start: number;
public end: number;

Expand All @@ -430,7 +433,7 @@ export class Span extends BaseNode {
}

export class Annotation extends SyntaxNode {
public type = "Annotation";
public type = "Annotation" as const;
public code: string;
public arguments: Array<unknown>;
public message: string;
Expand Down
28 changes: 15 additions & 13 deletions fluent-syntax/src/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* eslint no-magic-numbers: [0] */

import * as AST from "./ast.js";
// eslint-disable-next-line no-duplicate-imports
import type {Resource, Entry} from "./ast.js";
import { EOF, EOL, FluentParserStream } from "./stream.js";
import { ParseError } from "./errors.js";

Expand All @@ -12,10 +14,6 @@ type ParseFn<T> =
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(this: FluentParser, ps: FluentParserStream, ...args: Array<any>) => T;

type Comment = AST.Comment | AST.GroupComment | AST.ResourceComment
type Entry = AST.Message | AST.Term | Comment;
type EntryOrJunk = Entry | AST.Junk;

function withSpan<T extends AST.SyntaxNode>(fn: ParseFn<T>): ParseFn<T> {
return function (
this: FluentParser,
Expand Down Expand Up @@ -73,11 +71,11 @@ export class FluentParser {
/* eslint-enable @typescript-eslint/unbound-method */
}

parse(source: string): AST.Resource {
parse(source: string): Resource {
const ps = new FluentParserStream(source);
ps.skipBlankBlock();

const entries: Array<EntryOrJunk> = [];
const entries: Array<AST.Entry> = [];
let lastComment: AST.Comment | null = null;

while (ps.currentChar()) {
Expand Down Expand Up @@ -133,7 +131,7 @@ export class FluentParser {
* Preceding comments are ignored unless they contain syntax errors
* themselves, in which case Junk for the invalid comment is returned.
*/
parseEntry(source: string): EntryOrJunk {
parseEntry(source: string): Entry {
const ps = new FluentParserStream(source);
ps.skipBlankBlock();

Expand All @@ -149,7 +147,7 @@ export class FluentParser {
return this.getEntryOrJunk(ps);
}

getEntryOrJunk(ps: FluentParserStream): EntryOrJunk {
getEntryOrJunk(ps: FluentParserStream): AST.Entry {
const entryStartPos = ps.index;

try {
Expand Down Expand Up @@ -182,7 +180,7 @@ export class FluentParser {
}
}

getEntry(ps: FluentParserStream): Entry {
getEntry(ps: FluentParserStream): AST.Entry {
if (ps.currentChar() === "#") {
return this.getComment(ps);
}
Expand All @@ -198,7 +196,7 @@ export class FluentParser {
throw new ParseError("E0002");
}

getComment(ps: FluentParserStream): Comment {
getComment(ps: FluentParserStream): AST.Comments {
// 0 - comment
// 1 - group comment
// 2 - resource comment
Expand Down Expand Up @@ -669,7 +667,9 @@ export class FluentParser {
return selector;
}

getInlineExpression(ps: FluentParserStream): AST.Expression | AST.Placeable {
getInlineExpression(
ps: FluentParserStream
): AST.InlineExpression | AST.Placeable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (if-minor): this could be a bit cleaner if we separated getPlaceable from getInlineExpression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave this a look, and my intuition isn't strong here. And I don't think our tests are exhaustive. Like, I was rather surprised that

shared-photos = {NUMBER({1 ->
  *[1] 2
})}

works. Which made me wonder if it's not about expressions when it comes down to block vs inline, but placeable.

I've paged most of that context out of my brain, so it's not minor to me.

Playground also seems to behave differently to my local repo, and I'll call shotgun on that one. It still uses the 0.14 compat build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ouch. That's not spec compliant, right?

Spec doesn't allow for it - https://github.com/projectfluent/fluent/blob/master/spec/fluent.ebnf#L74
Rust doesn't allow for it - https://github.com/projectfluent/fluent-rs/blob/master/fluent-syntax/src/parser/expression.rs#L176

maybe it's time to consider Rust->JS export for the parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the spec allows it, https://github.com/projectfluent/fluent/blob/master/spec/fluent.ebnf#L59 has the inline_placeable as inline expression. And I tested the fluent-rs parser, too, and it was fine.

jazz = {{{1->
  *[1] 2
  }}->
  *[1] 3
}

OTH is illegal. Works on playground as in not only in the resolver, but also shows an AST.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, right. The placeholder loophole in inline :) retracting my previous statement.

if (ps.currentChar() === "{") {
return this.getPlaceable(ps);
}
Expand Down Expand Up @@ -736,7 +736,9 @@ export class FluentParser {
throw new ParseError("E0028");
}

getCallArgument(ps: FluentParserStream): AST.Expression | AST.NamedArgument {
getCallArgument(
ps: FluentParserStream
): AST.InlineExpression | AST.NamedArgument {
const exp = this.getInlineExpression(ps);

ps.skipBlank();
Expand All @@ -757,7 +759,7 @@ export class FluentParser {
}

getCallArguments(ps: FluentParserStream): AST.CallArguments {
const positional: Array<AST.Expression> = [];
const positional: Array<AST.InlineExpression> = [];
const named: Array<AST.NamedArgument> = [];
const argumentNames: Set<string> = new Set();

Expand Down
Loading