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

(Maybe String) parameters in API functions #24

Closed
a3f opened this issue Sep 9, 2014 · 18 comments

Comments

Projects
None yet
@a3f
Copy link
Contributor

commented Sep 9, 2014

There are quite some functions with optional LPTSTR parameters in the Winpi, where passing NULL invokes the default behaviour. An example for this is FindWindow (msdn).

The available Haskell wrappers don't support passing NULL to these functions.
The solution would be using Maybe String and writing a new withTString that returns nullPtr if Nothing is passed to it (i.e.: Maybe String -> (CString -> IO a) -> IO a). I can't find any readily available way to achieve the same, so I would make a group of functions à la withOptCWString and so on.

I'd be glad to give something back to the community by doing these changes, but as this also affects Foreign.C.String in ghc-base I wanted to ask for guidance first whether that's the right way to go :)

@ygale

This comment has been minimized.

Copy link

commented Sep 9, 2014

Thanks in advance for doing this!

It's a shame to have to add so many new functions to the API just for this. The type signatures for all those functions probably should have had Maybe String instead of String to begin with. But I agree, I don't see any choice at this point. Do you have an estimate for about how many new functions would need to be added to the API all together?

We should agree on a standard naming convention for API functions that take Maybe String instead of String for some of their arguments. E.g., something like findWindowOpt corresponding to findWindow, etc.

Some of those functions might use newTString instead of withTString. You'd need a new version of that, too.

I think you're right that newOptCString and friends ought to go in base. That should be discussed on the libraries@haskell.org mailing list. Reference this issue in your post. It could be that someone there knows some reason why this wasn't done to begin with.

Even if that does make it into base, you don't want the Win32 library to depend on a late version of base. So you would need to define those functions here also, probably in System.Win32.Types, wrapped in some CPP that defines them if the version of base is old and otherwise imports them.

@a3f

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2014

This search query leads me to believe there are a little under 150 such functions in the WinApi.

I am not sure if the Haskell wrapper supports them all though, but it wouldn't hurt adding them along the way if they're feasible to me.

I'll try to "cross-search" the results in the link above with the functions wrapped here and add an Opt version as first step as you suggested and raise a pull request.

Thank you for assistance :)

@mikesteele81

This comment has been minimized.

Copy link

commented Mar 17, 2015

@a3f,

In cases like your example of findWindow I usually re wrap the lower-level variant (c_findWindow in this case) within my own local Windows module. The higher-level function's source can be used as a template, so it's usually pretty straightforward.

I agree with you that it would be nice to have more convenient ways of supplying null pointers to common functions like this one.

@m37

This comment has been minimized.

Copy link

commented Mar 17, 2015

@a3f, @ygale,

I am wondering if it would be better to just change the signatures for these functions, instead of adding a whole lot of new, nearly-duplicate functions to accomplish this goal.

One reason (and a bit off-topic) for this is I noticed a very large number of functions (100+?) that have incorrect type signatures, usually due to using Int instead of CInt or another type, and also some more serious offenses. I was working on a pull request for that, about 1/2 done right now. Anyway, the only way to fix those is to change the type signatures (breaking backwards-compatibility), so why not just bite the bullet and do the same here?

@ndmitchell

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

My thoughts are similar to @m37 here, probably they should be Maybe String in the API properly. The hacky workarounds don't seem to make it that much easier to be compatible, and make it nasty forevermore. I think the number of people using Win32 directly is relatively low (in contrast to the vast number that use it indirectly) so fixing the relatively small number of people impacted should be feasible. [Disclaimer, I don't think it breaks any of my code, so I might be a little bit less concerned than I otherwise would be.]

@ygale

This comment has been minimized.

Copy link

commented Mar 18, 2015

@ndmitchell I also agree that the approach of @m37 is really the best, at least theoretically. We also would feel very little direct impact. But do you have any estimate of how much breakage there would likely be on Hackage, and in Windows builds of GHC, if we go that way?

@ndmitchell

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2015

On hackage there are 74 dependencies on Win32: http://packdeps.haskellers.com/reverse/Win32 - I guess a random sampling would probably give us the number that use relevant packages. No idea about GHC, but I suspect that's not too bad at all. Note that upgrading in a reverse compatible way should only be 5 lines, roughly:

#if Win32_min_version(2,5,0)
just = Just
#else
just = id
#endif

Then just add just in front of the necessary arguments.

@ygale

This comment has been minimized.

Copy link

commented Mar 18, 2015

Elliot Robertson posted this on the libraries list:

A quick reverse search [0] shows 74 packages having Win32 as a direct dependency, 3 of which are out of date. The big ones that stand out to me are directory (which will be up to Phil and me to fix) and process (which is maintained by Michael Snoyman (@snoyberg, ping)). Both are GHC dependencies, and each has 800+ reverse dependencies, so the release timing there will need to be well managed (possibly to coincide with the next major GHC RC?).

This being said, I'm definitely in favor of better Win32 support. If it takes some extra effort to fix long-standing defects, that's the price of doing business.

@snoyberg

This comment has been minimized.

Copy link

commented Mar 18, 2015

I don't have lots of experience using the Win32 library directly, though I have touched it a few times. I don't have a strong feeling on this issue, but I tend to agreeing with this being a change worth making (despite my normal hesitancy around breaking changes). If there's a branch of win32 available to test against, I'd be happy to test out process against it and see how much breakage ensues.

@ygale

This comment has been minimized.

Copy link

commented Mar 18, 2015

A few other important potentially affected packages that caught my eye:

  • cabal-install
  • haskeline
  • unix-compat
  • time
  • HTTP
@ygale

This comment has been minimized.

Copy link

commented Mar 18, 2015

I posted to the café and to reddit to make sure that everyone who needs to know about this will know. So far everyone seems to be positive, so I think it's time to work on getting an actual patch ready. @a3f and @m37, do you already have some work? Can you coordinate on it, or someone just do it?

@tejon

This comment has been minimized.

Copy link

commented Mar 18, 2015

In via reddit and for whatever my barely-invested newbie's opinion is worth, this seems like a great idea. I'd be on the fence if it were just the Maybe change, but @m37 pointing out those other type issues puts it over the top.

@m37

This comment has been minimized.

Copy link

commented Mar 19, 2015

@ygale,

I do have some code that I was working on, it's been a bit since I touched it but I think I'm about 1/2 way done. It doesn't include the changes to Maybe types like @a3f was working on, so I think if @a3f works on that part, and I continue on the rest, we probably won't overlap and our work would merge nicely. I'm open to other suggestions though.

@a3f

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2015

@ygale,

I also had some code done but then got lazy. I am willing to get this patch done though (I would dare say it mostly is).

I faced a design choice for which I need some input though:
In Foreign.C.String, we have:
peekCAString :: CString -> IO String
Now it ought to be either
peekCAString :: CString -> Maybe (IO String)
or
peekCAString :: CString -> IO (Maybe String)

Same goes for the other functions. Which one should we go for?
Maybe (IO String) makes the most sense, as there is no IO involved in case of a nullPtr, you just return Nothing.
IO (Maybe String) appears to be easier to work with.

I agree with @m37, I don't think that the eventual patches would interfere much with each other. I also agree on the point that the older signatures should be removed.

@mikesteele81
Ye, that's what I went with.

@rwbarton

This comment has been minimized.

Copy link

commented Mar 19, 2015

@a3f, I would go with the type signature that matches the function's intended use, namely peekCAString :: CString -> IO (Maybe String), particularly as, if someone needed the Maybe (IO String) version for some reason, it's quite a trivial function to recreate.

@hgolden

This comment has been minimized.

Copy link

commented Mar 20, 2015

Because this is a breaking change, I suggest keeping the existing Win32 as old-win32 for a couple of releases at least. I would expect the support cost to be small, since there would be little or no changes to the old (often incorrect) functions.

@3noch

This comment has been minimized.

Copy link

commented Mar 24, 2015

Just wanted to pitch my vote for the "breaking changes" approach. Haskell on Win32 is already a tad bit rough. If we pick an ugly backwards compatible approach, I deeply fear we would sacrifice "forward compatibility" (i.e. Win32 usage would only decrease because of superfluous complexity).

@Mistuke

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2016

Hmm does anyone know what ever happened to this proposal/change?

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 26, 2017

Nullable pointers now wrapped in Maybe (haskell#24)
A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe String for these arguments.

This breaks API. In total 14 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueEx

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 26, 2017

Nullable pointers now wrapped in Maybe (haskell#24)
A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 14 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueEx

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 26, 2017

Nullable pointers now wrapped in Maybe (haskell#24)
A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 14 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 26, 2017

Nullable pointers now wrapped in Maybe
A fix for Issue haskell#24

A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 15 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
setDllDirectory
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 27, 2017

Nullable pointers now wrapped in Maybe
A fix for Issue haskell#24

A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 15 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
setDllDirectory
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 27, 2017

Nullable pointers now wrapped in Maybe
A fix for Issue haskell#24

A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 15 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
setDllDirectory
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

a3f added a commit to a3f/ghc-win32 that referenced this issue Mar 27, 2017

Nullable pointers now wrapped in Maybe
A fix for Issue haskell#24

A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 15 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
setDllDirectory
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

Mistuke added a commit that referenced this issue Mar 28, 2017

Nullable pointers now wrapped in Maybe (#83)
A fix for Issue #24

A number of WinAPI functions have a default action,
when NULL is passed in place of a valid pointer, mostly strings.
We now use Maybe wrapped types for these arguments.

This breaks API. In total 15 functions were amended:
appendMenu
insertMenu
modifyMenu
messageBox
findWindow
findWindowEx
moveFileEx
setDllDirectory
defineDosDevice
setVolumeLabel
getVolumeInformation
searchPath
regCreateKeyEx
regReplaceKey
getTimeFormat

And 2 functions' Maybe was removed because passing in Nothing was equal to Just "":
RegQueryValue
RegQueryValueKey

createWindow[Ex] can also have NULL as HINSTANCE. As HINSTANCE is a pointer anyway,
we'll provide a nullHINSTANCE instead of breaking this function's API too

@Mistuke Mistuke closed this Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.