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.

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

Deprecate 'controller ... can' syntax #11363

Merged
merged 4 commits into from
Dec 1, 2021
Merged

Deprecate 'controller ... can' syntax #11363

merged 4 commits into from
Dec 1, 2021

Conversation

akrmn
Copy link
Contributor

@akrmn akrmn commented Oct 22, 2021 •
edited
Loading

DA GHC PR: digital-asset/ghc#79

Should close #11317

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description
  • If you mean to change the status of a component, please make sure you keep the Component Status page up to date.

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@moisesackerman-da moisesackerman-da force-pushed the ghc-deprecate-controller-can branch 5 times, most recently from 87b9546 to 6135604 Compare October 25, 2021 15:09
@akrmn akrmn marked this pull request as ready for review October 25, 2021 15:09
@akrmn akrmn requested a review from a user October 25, 2021 15:09
@moisesackerman-da moisesackerman-da force-pushed the ghc-deprecate-controller-can branch 2 times, most recently from 88e1a5a to c7e94c4 Compare October 25, 2021 15:47
@akrmn akrmn changed the title Add warning for uses of 'controller ... can' syntax Deprecate 'controller ... can' syntax Oct 25, 2021
ghost
ghost approved these changes Oct 25, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

language-support/java/codegen/src/ledger-tests/daml/Stakeholders.daml Outdated Show resolved Hide resolved
compiler/damlc/tests/daml-test-files/SemanticsEvalOrder.daml Outdated Show resolved Hide resolved
cocreature
cocreature approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Great work thank you!

A few minor comments + 2 things:

  1. Note that we only want this in Daml 2.0 so this needs to be merged only after 1.18.0 (probably mid next week so you don’t have to wait long).
  2. This should have a changelog entry mentioning the implicit observer behavior.

ci/da-ghc-lib/compile.yml Outdated Show resolved Hide resolved
compiler/damlc/tests/daml-test-files/SemanticsEvalOrder.daml Outdated Show resolved Hide resolved
language-support/java/codegen/src/ledger-tests/daml/Stakeholders.daml Outdated Show resolved Hide resolved
@moisesackerman-da moisesackerman-da force-pushed the ghc-deprecate-controller-can branch 3 times, most recently from 431ec6a to b638d26 Compare October 26, 2021 13:33
@akrmn akrmn changed the title Deprecate 'controller ... can' syntax [DO NOT MERGE] Deprecate 'controller ... can' syntax Oct 26, 2021
@akrmn
Copy link
Contributor Author

akrmn commented Oct 26, 2021 •
edited
Loading

Current status:

  • Merged the GHC PR
  • Bumped the patches line of ci/da-ghc-lib/compile.yml.
  • Added the requested comments
  • Added a test for the warning
  • Added {-# OPTIONS_GHC -Wno-controller-can #-} for the daml files which still contain the deprecated syntax.
  • Updated stack-snapshot.yaml and stack_snapshot_windows.yaml

I also prefixed the PR name with [DO NOT MERGE] and added the don't merge! label so we remember not to merge before 1.18.0.

@moisesackerman-da moisesackerman-da force-pushed the ghc-deprecate-controller-can branch from 5fb9ae0 to 9d1f3b0 Compare October 26, 2021 14:10
@akrmn
Copy link
Contributor Author

akrmn commented Oct 26, 2021

Failed on daml/TestInterfaces.daml, which seems unrelated.

File:     daml/TestInterfaces.daml
Hidden:   no
Range:    1:1-1:1
Source:   typecheck
Severity: DsError
Message: 
  daml/TestInterfaces.daml:1:1: error:
  Not in scope:
  type constructor or class ‘DA.Internal.Desugar.TypeRep’
  Perhaps you meant one of these:
  ‘DA.Internal.Desugar.Text’ (imported from DA.Internal.Desugar),
  ‘DA.Internal.Desugar.Update’ (imported from DA.Internal.Desugar),
  ‘DA.Internal.Desugar.Choice’ (imported from DA.Internal.Desugar)
  Module ‘DA.Internal.Desugar’ does not export ‘TypeRep’.
ERROR: Creation of DAR file failed.
Compiling script-test-1dev to a DAR.
WARNING: Package names should have the format ^[A-Za-z][A-Za-z0-9]*(\-[A-Za-z][A-Za-z0-9]*)*$.
You may be able to compile packages with different formats, but you will not be able to
use them as dependencies in other projects. Unsupported package names or versions may
start causing compilation errors without warning.

@cocreature
Copy link
Contributor

Can you try rebasing? That looks like it might be due to some weird inconsistency with the work Robin has been doing.

@moisesackerman-da moisesackerman-da force-pushed the ghc-deprecate-controller-can branch from 9d1f3b0 to da2621d Compare October 28, 2021 08:38
@cocreature cocreature mentioned this pull request Oct 29, 2021
Closed
7 tasks
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch from 2442a9c to 3f738bf Compare November 19, 2021 15:22
@akrmn akrmn changed the title [DO NOT MERGE] Deprecate 'controller ... can' syntax Deprecate 'controller ... can' syntax Nov 19, 2021
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch 2 times, most recently from e571cc2 to bed170a Compare November 19, 2021 15:57
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch 3 times, most recently from 8434ab1 to d3e7633 Compare November 30, 2021 15:50
@akrmn akrmn changed the base branch from main to ghc-lib-haskell-deps November 30, 2021 15:51
@akrmn akrmn force-pushed the ghc-lib-haskell-deps branch from 51181ca to c182092 Compare November 30, 2021 15:56
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch from d3e7633 to 53bd98a Compare November 30, 2021 15:57
Base automatically changed from ghc-lib-haskell-deps to main November 30, 2021 18:11
cocreature
cocreature reviewed Dec 1, 2021
View reviewed changes
bazel-haskell-deps.bzl Outdated Show resolved Hide resolved
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch 3 times, most recently from fb52dd2 to 91c9eb5 Compare December 1, 2021 09:42
@akrmn akrmn removed the don't merge! label Dec 1, 2021
akrmn added 4 commits December 1, 2021 11:34
@akrmn
Add warning for uses of 'controller ... can' syntax
1c9aee4 
part of #11317

changelog_begin
* Deprecate 'controller ... can' syntax.
  * It will be removed in a future version of Daml.
  * Instead, use 'choice ... with ... controller' syntax. Note that this does not implictly add the controller as an observer, so it must be added explictly as one (or as a signatory).
changelog_end
@akrmn
Remove uses of 'controller ... can' syntax
b5d3419 
part of #11317
@akrmn
Update docs to reflect 'controller ... can' syntax deprecation
4f4d9df 
Also fixed some literalinclude snippets to use blocks instead of line numbers

part of #11317
@akrmn
Add test cases for -W{,no-}controller-can
f3ec1cc
@akrmn akrmn force-pushed the ghc-deprecate-controller-can branch from 91c9eb5 to f3ec1cc Compare December 1, 2021 10:34
@akrmn akrmn merged commit 0b79d8a into main Dec 1, 2021
@akrmn akrmn deleted the ghc-deprecate-controller-can branch December 1, 2021 12:36
@akrmn akrmn mentioned this pull request Sep 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cocreature cocreature cocreature approved these changes

@aherrmann-da aherrmann-da Awaiting requested review from aherrmann-da

@bame-da bame-da Awaiting requested review from bame-da

@garyverhaegen-da garyverhaegen-da Awaiting requested review from garyverhaegen-da

@nemanja-da nemanja-da Awaiting requested review from nemanja-da

@nickchapman-da nickchapman-da Awaiting requested review from nickchapman-da

@S11001001 S11001001 Awaiting requested review from S11001001

@stefanobaghino-da stefanobaghino-da Awaiting requested review from stefanobaghino-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Emit warnings on controller can syntax
2 participants
@akrmn @cocreature