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

fix #13790; ptr char (+friends) should not implicitly convert to cstring #20761

Merged
merged 19 commits into from
Nov 24, 2022
Merged

fix #13790; ptr char (+friends) should not implicitly convert to cstring #20761

merged 19 commits into from
Nov 24, 2022

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 5, 2022 •
edited
Loading

fix #13790
ref timotheecour@b9ecff1

Explicit conversion is disabled in this PR too, since the conversion is unsafe.

It breaks at compile time.

@ringabout ringabout changed the title ptr char (+friends) should not implicitly convert to cstring fix #13790; ptr char (+friends) should not implicitly convert to cstring Nov 5, 2022
@arnetheduck
Copy link
Contributor

might be a good idea to add a warning + a strict mode instead

ringabout
ringabout commented Nov 5, 2022
View reviewed changes
compiler/sigmatch.nim Outdated Show resolved Hide resolved
@ringabout
Copy link
Member Author

might be a good idea to add a warning + a strict mode instead

I agree I will hide it behind nimPreviewSlimSystem and gives a warning in lenient mode.

@arnetheduck
Copy link
Contributor

nimPreviewSlimSystem

is the slim system scheduled to become enabled by default?

@ringabout
Copy link
Member Author

ringabout commented Nov 11, 2022 •
edited
Loading

I'm not sure whether it has been decided that nimPreviewSlimSystem will be enabled by default for v2.

It has been enabled for compiler, tests/stdlib, and documentation builds (not tests). If it is going to be enabled for v2, testament will be patched to make important packages' tests green.

@ringabout ringabout marked this pull request as ready for review November 11, 2022 07:24
@Araq
Copy link
Member

Araq commented Nov 11, 2022 •
edited
Loading

is the slim system scheduled to become enabled by default?

No. v2 is mostly about finally making mm:orc the default plus a couple of other things that we consider production ready enough for v2. These are features that exist at least in 1.6 already.

There are new switches like slim system and "strictDefs" and "strictCaseObjects" which have seen their introduction with v2 and thus are feature previews.

@arnetheduck
Copy link
Contributor

in that case, I believe this fix should go under a separate flag + that the warning should be available on 1.6 too (so that people can find the implicit conversions in their code starting now) - with the warning in place, it becomes trivial to fix before this becomes enforced

@arnetheduck
Copy link
Contributor

also, it would be nice to be able to set the "explicitness" of any compiler message (ie silent/hint/warning/error), so that we can do --msg[PtrToCstringConv]=warning while working on it then --msg[PtrToCstringConv]=error when we've fixed all existing code and want to put a hard block on new code re-introducing it

@ringabout ringabout mentioned this pull request Nov 11, 2022
Merged
@ringabout
Copy link
Member Author

done => #20814

@ringabout ringabout marked this pull request as draft November 12, 2022 14:40
@ringabout ringabout marked this pull request as ready for review November 13, 2022 02:07
@ringabout
Copy link
Member Author

cast[cstring] refactor will be done later. I will examine some cases first.

@ringabout ringabout marked this pull request as draft November 13, 2022 03:57
@ringabout ringabout marked this pull request as ready for review November 13, 2022 05:34
@ringabout
Copy link
Member Author

in that case, I believe this fix should go under a separate flag + that the warning should be available on 1.6 too

Done; I think this PR should be backported too.

@ringabout
Copy link
Member Author

ringabout commented Nov 14, 2022 •
edited
Loading

Compiler and stdlib fixes should be backported at least.

@Varriount Varriount requested a review from Araq November 14, 2022 18:46
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Nov 14, 2022
@ringabout ringabout mentioned this pull request Nov 15, 2022
Merged
@planetis-m planetis-m mentioned this pull request Nov 15, 2022
Merged
@Araq Araq merged commit 27a38a9 into devel Nov 24, 2022
@Araq Araq deleted the pr_ctring_ptr branch November 24, 2022 06:49
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 27a38a9

Hint: mm: orc; opt: speed; options: -d:release
165220 lines; 8.632s; 614.418MiB peakmem

@metagn metagn mentioned this pull request Nov 27, 2022
Closed
survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
@ringabout @survivorm
fix nim-lang#13790; ptr char (+friends) should not implicitly convert…
454e57e 
… to cstring (nim-lang#20761)

* fix =nim-lang#13790 ptr char (+friends) should not implicitly convert to cstring

* Apply suggestions from code review

* first round; compiles on windows

* nimPreviewSlimSystem

* conversion is unsafe, cast needed

* fixes more tests

* fixes asyncnet

* another try another error

* last one

* true

* one more

* why bugs didn't show at once

* add `nimPreviewCstringConversion` switch

* typo

Co-authored-by: xflywind <43030857+xflywind@users.noreply.github.com>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
@ringabout @capocasa
fix nim-lang#13790; ptr char (+friends) should not implicitly convert…
b0283ed 
… to cstring (nim-lang#20761)

* fix =nim-lang#13790 ptr char (+friends) should not implicitly convert to cstring

* Apply suggestions from code review

* first round; compiles on windows

* nimPreviewSlimSystem

* conversion is unsafe, cast needed

* fixes more tests

* fixes asyncnet

* another try another error

* last one

* true

* one more

* why bugs didn't show at once

* add `nimPreviewCstringConversion` switch

* typo

Co-authored-by: xflywind <43030857+xflywind@users.noreply.github.com>
@metagn metagn mentioned this pull request May 29, 2023
Merged
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
@ringabout @bung87
fix nim-lang#13790; ptr char (+friends) should not implicitly convert…
907c847 
… to cstring (nim-lang#20761)

* fix =nim-lang#13790 ptr char (+friends) should not implicitly convert to cstring

* Apply suggestions from code review

* first round; compiles on windows

* nimPreviewSlimSystem

* conversion is unsafe, cast needed

* fixes more tests

* fixes asyncnet

* another try another error

* last one

* true

* one more

* why bugs didn't show at once

* add `nimPreviewCstringConversion` switch

* typo

Co-authored-by: xflywind <43030857+xflywind@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Awaiting requested review from Araq

Assignees
No one assigned
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ptr char implicitly converts to cstring, resulting in undefined behavior
4 participants
@ringabout @arnetheduck @Araq @Varriount