Skip to content
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.

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

Identifiers that are reserved keywords should be disallowed. #305

Open
claymcleod opened this issue Jan 22, 2025 · 4 comments
Open

Identifiers that are reserved keywords should be disallowed. #305

claymcleod opened this issue Jan 22, 2025 · 4 comments
Assignees
Labels
spec compliance Behavior is not to the WDL specification

Comments

@claymcleod
Copy link
Member

The specification says (link):

The following (case-sensitive) language keywords are reserved and cannot be used to name declarations, calls, tasks, workflows, import namespaces, struct types, or aliases.

However, wdl is completely fine with reserved keywords being used as identifiers.

version 1.2

task run {
    input {
        String in
    }
}

This should be a validation error instead.

@peterhuene
Copy link
Collaborator

peterhuene commented Jan 22, 2025

The reading of declarations in that context is ambiguous to me.

At the time this was implemented, it was assumed the specification meant private declarations and therefore it allows reserved WDL keywords in task/workflow inputs and outputs.

Thus the places we currently accept reserved names is:

  • Struct member names and (by extension) literal struct expression key names
  • Task and workflow input and output names
  • Runtime/hints/requirement key names
  • Metadata and parameter metadata key names
  • Call inputs (necessary because reserved names are allowed for inputs)
  • Name reference expressions (necessary to reference inputs and outputs)

We should get clarification from the spec.

For what it's worth, miniwdl does not fail to validate it either (but fails to parse for other keywords).

@claymcleod
Copy link
Member Author

claymcleod commented Jan 23, 2025

Yeah that makes sense. When you go look at the spec section on declarations, I feel that it's clear that unbound input/output declarations are included in that phrasing (e.g., the first example there in the spec specifically uses input declarations within their illustrative example). If you still feel that it needs clarification after re-reading that section, I can make an issue.

EDIT: I further see this now in the declarations section right above where the example with input/output declarations is used:

A declaration reserves a name that can be referenced anywhere in the scope where it is declared. A declaration has a type, a name, and an optional initialization. Each declaration must be unique within its scope, and may not collide with a reserved WDL keyword (e.g., workflow, or input).

Now I'm convinced there's really no room for allowing this. Let me know if you feel otherwise.

@peterhuene
Copy link
Collaborator

peterhuene commented Jan 23, 2025

Agreed, that language is not ambiguous. I'll fix this.

The spec also uses "declarations" nomenclature around struct members, so I'm going to extend the prohibition on reserved keywords there as well.

@peterhuene peterhuene self-assigned this Jan 23, 2025
@peterhuene peterhuene added the spec compliance Behavior is not to the WDL specification label Jan 23, 2025
@claymcleod
Copy link
Member Author

Agreed, that language is not ambiguous. I'll fix this.

The spec also uses "declarations" nomenclature around struct members, so I'm going to extend the prohibition on reserved keywords there as well.

Yeah that sounds correct. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec compliance Behavior is not to the WDL specification
Projects
None yet
Development

No branches or pull requests

2 participants