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

Relax uppercase check for types qualified with a namespace #1493

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

romac
Copy link
Member

@romac romac commented Sep 2, 2024

Closes: #1494

Allows type foo::Bar = int whereas it would previously fail with type names must start with an uppercase letter because the check was looking at the very first letter of the fully qualified name (ie. foo::Bar) instead of the unqualified name (ie. Bar).

Example

module upper {
  type Foo = int
  type foo = int

  type foo::Bar = int
  type foo::bar = int
}

Before

upper.qnt:2:3 - error: [QNT007] type names must start with an uppercase letter
2:   type Foo = int
     ^^^^^^^^^^^^^^

upper.qnt:3:3 - error: [QNT007] type names must start with an uppercase letter
3:   type foo = int
     ^^^^^^^^^^^^^^

upper.qnt:5:3 - error: [QNT007] type names must start with an uppercase letter
5:   type foo::Bar = int
     ^^^^^^^^^^^^^^^^^^^

upper.qnt:6:3 - error: [QNT007] type names must start with an uppercase letter
6:   type foo::bar = int
     ^^^^^^^^^^^^^^^^^^^

error: parsing failed

After

/Users/romac/Informal/Code/quint/quint/upper.qnt:3:3 - error: [QNT007] type names must start with an uppercase letter
3:   type foo = int
     ^^^^^^^^^^^^^^

/Users/romac/Informal/Code/quint/quint/upper.qnt:5:3 - error: [QNT007] type names must start with an uppercase letter
5:   type foo::bar = int
     ^^^^^^^^^^^^^^^^^^^

error: parsing failed

  • Tests added for any new code
  • Documentation added for any new functionality
  • Entries added to the respective CHANGELOG.md for any new functionality
  • Feature table on README.md updated for any listed functionality

Allows `type foo::Bar = int` whereas it would previously fail
with `type names must start with an uppercase letter` because the
check was looking at the very first letter of the fully qualified
name (ie. `foo::Bar`) instead of the unqualified name (ie. `Bar`).
@romac romac marked this pull request as ready for review September 2, 2024 16:25
@romac romac requested a review from bugarela September 2, 2024 16:25
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

This is great! I think we can easily add a unit test for this: we can just add a type declaration with the new supported name pattern to SuperSpec.qnt and then update the expected fixtures with npm run update-fixtures. If all works well, the new fixture will be only slightly different, but still a success - while before, a type like that would cause errors. It is not a really good unit test setup, but at least it will prevent us from regressing from this.

quint/src/parsing/parseErrors.ts Show resolved Hide resolved
@romac
Copy link
Member Author

romac commented Sep 3, 2024

On CI, the quint-test-generated-up-to-date is failing with

ERROR: Generated files are not up to date. Run 'npm run generate'

But when I run the suggested command I don't see any changes in my working copy

@bugarela
Copy link
Collaborator

bugarela commented Sep 3, 2024

On CI, the quint-test-generated-up-to-date is failing with

ERROR: Generated files are not up to date. Run 'npm run generate'

But when I run the suggested command I don't see any changes in my working copy

Unfortunately this command (or the npm run update-fixtures which is part of it) generates also files that are not relevant to us, and are not tracked by git, so what I do is not commit them and run git clean -f testFixture/* to remove them from my working copy. I see that you committed those files in your last commit, and that's is likely what is causing the CI to break.

Can you remove those extra files? All of them but the SuperSpec* ones. Then I think it will work.

@romac romac force-pushed the romac/qualified-type branch from 3c0e44c to 56976cf Compare September 3, 2024 15:07
@romac romac requested a review from bugarela September 3, 2024 15:40
@romac
Copy link
Member Author

romac commented Sep 3, 2024

I see that you committed those files in your last commit

Indeed, my bad sorry! Fixed now.

Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

Amazing, thanks again!

@romac romac merged commit ae27080 into main Sep 4, 2024
14 checks passed
@romac romac deleted the romac/qualified-type branch September 4, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types prefixed with a namespace yield QNT007 error
2 participants